From 981726cc15511a95c7089776b187eb226f9b4573 Mon Sep 17 00:00:00 2001 From: Ben Thomas Date: Wed, 13 May 2026 12:03:27 -0700 Subject: [PATCH 1/7] .NET: feat(evals): add ground_truth/expected_output support for workflow evaluation (#5755) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * .NET: feat(evals): add ground_truth/expected_output support for workflow eval Brings .NET to parity with Python PR #5234 for issue #5135: - Add expectedOutput parameter to Run.EvaluateAsync (workflow) and stamp on the overall EvalItem.ExpectedOutput. - Map EvalItem.ExpectedOutput -> ground_truth in the Foundry JSONL payload, item_schema, and data_mapping for similarity. - Add GroundTruthEvaluators set (currently builtin.similarity) and a FindMissingGroundTruthEvaluators helper. - Fail fast with InvalidOperationException when a ground-truth evaluator is selected but no item provides an ExpectedOutput, instead of surfacing a remote provider error. - Add tests in FoundryEvalConverterTests and WorkflowEvaluationTests. - Add Evaluation_WorkflowExpectedOutputs sample (workflow + Foundry similarity). Fixes microsoft/agent-framework#5135 (.NET side). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address review: relax BuildOverallItem events to IReadOnlyList Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Sample: disable per-agent breakdown when using reference-based evaluator Per-agent EvalItems are intentionally left without ExpectedOutput, so the new fail-fast validation in FoundryEvals would throw when Similarity is invoked for per-agent items. Pass includePerAgent: false in the workflow + similarity sample, and document this gotcha in the EvaluateAsync XML doc. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix BuildOverallItem: fall back to last ExecutorCompletedEvent AgentResponseEvent is only emitted when AIAgentHostOptions.EmitAgentResponseEvents is enabled, which is not the default for WorkflowBuilder(agent).AddEdge(...). When it is absent, fall back to the last non-internal ExecutorCompletedEvent whose Data is an AgentResponse / ChatMessage / string so the overall EvalItem (and any expectedOutput) is produced. Without this, samples wired up the standard way returned 0 evaluation items. Update test to cover the fallback path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Sample: enable EmitAgentResponseEvents; eval throws clear error when no overall response found Root cause of '0 results': AIAgentHostExecutor only emits AgentResponseEvent when AIAgentHostOptions.EmitAgentResponseEvents is true (default false). For ordinary AIAgent executors the runtime's ExecutorCompletedEvent.Data is null, so the prior fallback couldn't find a final response either. Sample now builds executors with EmitAgentResponseEvents=true via BindAsExecutor(hostOptions). EvaluateAsync now throws InvalidOperationException with a remediation hint when the user supplies expectedOutput but no overall final response can be located, instead of silently returning 0/0. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Guard against null sample/error/usage/datasource_item in ParseDetailedItem Foundry eval responses can have these properties present with JSON null or non-object values, which caused JsonElement.TryGetProperty to throw 'requires Object, has Null'. Check ValueKind == Object before drilling in. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address PR review: reorder expectedOutput, tighten ground-truth check, add fail-fast test * WorkflowEvaluationExtensions.EvaluateAsync: move 'expectedOutput' to after 'splitter' so the original positional contract of (splitter, cancellationToken) is preserved for existing callers. * FoundryEvals: require ALL items to carry ExpectedOutput when a ground-truth evaluator is selected (e.g. similarity), not just any. Reference-based evaluators score per-item, so a single missing GT would still surface as a provider-side validation error. Updated fail-fast message accordingly. * WorkflowEvaluationTests: add EvaluateAsync_WithExpectedOutputButNoFinalResponse_ThrowsAsync to verify the InvalidOperationException is thrown (and that the message mentions EmitAgentResponseEvents). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fail-fast on missing overall item regardless of expectedOutput; harden BuildOverallItem default * EvaluateAsync now throws InvalidOperationException whenever 'includeOverall' is requested but BuildOverallItem cannot produce an item, instead of only when 'expectedOutput' is supplied. Same misconfiguration (agents not bound with EmitAgentResponseEvents) used to silently return empty results — now it surfaces a clear, actionable error in both cases. * BuildOverallItem switch default now throws instead of returning null. The preceding for-loop already constrains Data to AgentResponse/ChatMessage/ string, so reaching default would indicate a contract drift; throw to make the bug visible. * Test renamed and broadened to verify the throw fires without expectedOutput. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: alliscode <25218250+alliscode@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- dotnet/agent-framework-dotnet.slnx | 1 + .../Evaluation_WorkflowExpectedOutputs.csproj | 16 +++ .../Program.cs | 76 ++++++++++ .../README.md | 37 +++++ .../Evaluation/FoundryEvalConverter.cs | 44 +++++- .../Evaluation/FoundryEvalWireModels.cs | 3 + .../Evaluation/FoundryEvals.cs | 26 +++- .../WorkflowEvaluationExtensions.cs | 132 +++++++++++++++--- .../FoundryEvalConverterTests.cs | 109 +++++++++++++++ .../InputWaiterAndOutputFilterTests.cs | 13 +- .../WorkflowEvaluationTests.cs | 116 +++++++++++++++ 11 files changed, 542 insertions(+), 31 deletions(-) create mode 100644 dotnet/samples/03-workflows/Evaluation/Evaluation_WorkflowExpectedOutputs/Evaluation_WorkflowExpectedOutputs.csproj create mode 100644 dotnet/samples/03-workflows/Evaluation/Evaluation_WorkflowExpectedOutputs/Program.cs create mode 100644 dotnet/samples/03-workflows/Evaluation/Evaluation_WorkflowExpectedOutputs/README.md diff --git a/dotnet/agent-framework-dotnet.slnx b/dotnet/agent-framework-dotnet.slnx index c684fcf883..87e6d9d3c6 100644 --- a/dotnet/agent-framework-dotnet.slnx +++ b/dotnet/agent-framework-dotnet.slnx @@ -298,6 +298,7 @@ + diff --git a/dotnet/samples/03-workflows/Evaluation/Evaluation_WorkflowExpectedOutputs/Evaluation_WorkflowExpectedOutputs.csproj b/dotnet/samples/03-workflows/Evaluation/Evaluation_WorkflowExpectedOutputs/Evaluation_WorkflowExpectedOutputs.csproj new file mode 100644 index 0000000000..adbcde8572 --- /dev/null +++ b/dotnet/samples/03-workflows/Evaluation/Evaluation_WorkflowExpectedOutputs/Evaluation_WorkflowExpectedOutputs.csproj @@ -0,0 +1,16 @@ + + + + Exe + net10.0 + + enable + enable + + + + + + + + diff --git a/dotnet/samples/03-workflows/Evaluation/Evaluation_WorkflowExpectedOutputs/Program.cs b/dotnet/samples/03-workflows/Evaluation/Evaluation_WorkflowExpectedOutputs/Program.cs new file mode 100644 index 0000000000..30fa79faa8 --- /dev/null +++ b/dotnet/samples/03-workflows/Evaluation/Evaluation_WorkflowExpectedOutputs/Program.cs @@ -0,0 +1,76 @@ +// Copyright (c) Microsoft. All rights reserved. + +// This sample demonstrates evaluating a multi-agent workflow against a +// golden answer using Foundry's reference-based Similarity evaluator. + +using Azure.AI.Projects; +using Azure.Identity; +using Microsoft.Agents.AI; +using Microsoft.Agents.AI.Workflows; +using Microsoft.Extensions.AI; +using FoundryEvals = Microsoft.Agents.AI.Foundry.FoundryEvals; + +string endpoint = Environment.GetEnvironmentVariable("AZURE_AI_PROJECT_ENDPOINT") + ?? throw new InvalidOperationException("AZURE_AI_PROJECT_ENDPOINT is not set."); +string deploymentName = Environment.GetEnvironmentVariable("AZURE_AI_MODEL_DEPLOYMENT_NAME") ?? "gpt-4o-mini"; + +// WARNING: DefaultAzureCredential is convenient for development but requires careful consideration in production. +// In production, consider using a specific credential (e.g., ManagedIdentityCredential) to avoid +// latency issues, unintended credential probing, and potential security risks from fallback mechanisms. +AIProjectClient projectClient = new(new Uri(endpoint), new DefaultAzureCredential()); + +// Build a two-agent workflow: a researcher writes a draft answer, then an +// editor polishes it into the final response that we compare to ground truth. +// EmitAgentResponseEvents is enabled so the workflow surfaces an AgentResponseEvent +// for each agent — this is what EvaluateAsync uses to find the overall final answer. +var hostOptions = new AIAgentHostOptions { EmitAgentResponseEvents = true }; + +AIAgent researcher = projectClient.AsAIAgent( + model: deploymentName, + instructions: "You research questions and produce a short factual draft answer.", + name: "researcher"); + +AIAgent editor = projectClient.AsAIAgent( + model: deploymentName, + instructions: "You take a draft answer and produce the final concise response.", + name: "editor"); + +ExecutorBinding researcherExecutor = researcher.BindAsExecutor(hostOptions); +ExecutorBinding editorExecutor = editor.BindAsExecutor(hostOptions); + +Workflow workflow = new WorkflowBuilder(researcherExecutor) + .AddEdge(researcherExecutor, editorExecutor) + .Build(); + +// Run the workflow against the user question. +const string Query = "What is the capital of France?"; +const string GroundTruth = "Paris"; + +await using Run run = await InProcessExecution.RunAsync( + workflow, + new ChatMessage(ChatRole.User, Query)); + +// Evaluate the overall workflow output against a golden answer using the +// reference-based Similarity evaluator. The 'expectedOutput' value is stamped +// onto the overall EvalItem.ExpectedOutput and is surfaced to Foundry as +// `ground_truth` in the underlying JSONL payload. +// +// Per-agent breakdown is disabled here: ground truth applies to the workflow's +// final answer, not to each sub-agent's intermediate output. Without +// includePerAgent: false, the evaluator would be invoked for per-agent items +// (which have no ExpectedOutput) and Similarity would fail validation. +FoundryEvals similarity = new(projectClient, deploymentName, FoundryEvals.Similarity); + +AgentEvaluationResults results = await run.EvaluateAsync( + similarity, + includePerAgent: false, + expectedOutput: GroundTruth); + +Console.WriteLine($"Query: {Query}"); +Console.WriteLine($"Expected: {GroundTruth}"); +Console.WriteLine($"Provider: {results.ProviderName}"); +Console.WriteLine($"Passed: {results.Passed}/{results.Total}"); +if (results.ReportUrl is not null) +{ + Console.WriteLine($"Report: {results.ReportUrl}"); +} diff --git a/dotnet/samples/03-workflows/Evaluation/Evaluation_WorkflowExpectedOutputs/README.md b/dotnet/samples/03-workflows/Evaluation/Evaluation_WorkflowExpectedOutputs/README.md new file mode 100644 index 0000000000..9390e91e4c --- /dev/null +++ b/dotnet/samples/03-workflows/Evaluation/Evaluation_WorkflowExpectedOutputs/README.md @@ -0,0 +1,37 @@ +# Evaluation - Workflow Expected Outputs + +This sample demonstrates evaluating a multi-agent workflow's final answer +against a golden expected output using Foundry's reference-based **Similarity** +evaluator. + +## What this sample demonstrates + +- Building a small researcher → editor workflow +- Running the workflow and obtaining a `Run` +- Calling `run.EvaluateAsync(evaluator, expectedOutput: ...)` to attach a + ground-truth answer to the overall workflow item +- Using `FoundryEvals.Similarity`, which requires a `ground_truth` value + per item + +The `expectedOutput` value is stamped onto the overall `EvalItem.ExpectedOutput` +and is surfaced to Foundry as `ground_truth` in the JSONL payload sent to the +Evals API. + +## Prerequisites + +- .NET 10 SDK or later +- Azure CLI installed and authenticated (`az login`) + +Set the following environment variables: + +```powershell +$env:AZURE_AI_PROJECT_ENDPOINT="https://your-foundry-service.services.ai.azure.com/api/projects/your-foundry-project" +$env:AZURE_AI_MODEL_DEPLOYMENT_NAME="gpt-4o-mini" +``` + +## Run the sample + +```powershell +cd dotnet/samples/03-workflows/Evaluation +dotnet run --project .\Evaluation_WorkflowExpectedOutputs +``` diff --git a/dotnet/src/Microsoft.Agents.AI.Foundry/Evaluation/FoundryEvalConverter.cs b/dotnet/src/Microsoft.Agents.AI.Foundry/Evaluation/FoundryEvalConverter.cs index c539175ed2..0754e2bc76 100644 --- a/dotnet/src/Microsoft.Agents.AI.Foundry/Evaluation/FoundryEvalConverter.cs +++ b/dotnet/src/Microsoft.Agents.AI.Foundry/Evaluation/FoundryEvalConverter.cs @@ -130,6 +130,7 @@ internal static class FoundryEvalConverter QueryMessages = ConvertMessages(queryMessages), ResponseMessages = ConvertMessages(responseMessages), Context = item.Context, + GroundTruth = item.ExpectedOutput, ToolDefinitions = item.Tools is { Count: > 0 } ? item.Tools .OfType() @@ -185,6 +186,11 @@ internal static class FoundryEvalConverter dataMapping["context"] = "{{item.context}}"; } + if (GroundTruthEvaluators.Contains(qualified)) + { + dataMapping["ground_truth"] = "{{item.ground_truth}}"; + } + if (ToolEvaluators.Contains(qualified)) { dataMapping["tool_definitions"] = "{{item.tool_definitions}}"; @@ -206,7 +212,7 @@ internal static class FoundryEvalConverter /// /// Builds the item_schema for custom JSONL eval definitions. /// - internal static WireItemSchema BuildItemSchema(bool hasContext = false, bool hasTools = false) + internal static WireItemSchema BuildItemSchema(bool hasContext = false, bool hasTools = false, bool hasGroundTruth = false) { var properties = new Dictionary { @@ -221,6 +227,11 @@ internal static class FoundryEvalConverter properties["context"] = new WireSchemaProperty { Type = "string" }; } + if (hasGroundTruth) + { + properties["ground_truth"] = new WireSchemaProperty { Type = "string" }; + } + if (hasTools) { properties["tool_definitions"] = new WireSchemaProperty { Type = "array" }; @@ -233,6 +244,31 @@ internal static class FoundryEvalConverter }; } + /// + /// Returns the subset of that require a ground-truth + /// (reference) value but cannot be evaluated because no item provided one. + /// + internal static List FindMissingGroundTruthEvaluators( + IEnumerable evaluators, + bool hasGroundTruth) + { + if (hasGroundTruth) + { + return []; + } + + var missing = new List(); + foreach (var name in evaluators) + { + if (GroundTruthEvaluators.Contains(ResolveEvaluator(name))) + { + missing.Add(name); + } + } + + return missing; + } + /// /// Resolves a short evaluator name to its fully-qualified builtin.* form. /// @@ -277,6 +313,12 @@ internal static class FoundryEvalConverter "builtin.tool_call_success", }; + // Evaluators that require a ground_truth (reference) value per item. + internal static readonly HashSet GroundTruthEvaluators = new(StringComparer.OrdinalIgnoreCase) + { + "builtin.similarity", + }; + // Short name → fully-qualified name mapping. internal static readonly Dictionary BuiltinEvaluators = new(StringComparer.OrdinalIgnoreCase) { diff --git a/dotnet/src/Microsoft.Agents.AI.Foundry/Evaluation/FoundryEvalWireModels.cs b/dotnet/src/Microsoft.Agents.AI.Foundry/Evaluation/FoundryEvalWireModels.cs index 4438b35807..c05232575c 100644 --- a/dotnet/src/Microsoft.Agents.AI.Foundry/Evaluation/FoundryEvalWireModels.cs +++ b/dotnet/src/Microsoft.Agents.AI.Foundry/Evaluation/FoundryEvalWireModels.cs @@ -103,6 +103,9 @@ internal sealed class WireEvalItemPayload [JsonPropertyName("context")] public string? Context { get; init; } + [JsonPropertyName("ground_truth")] + public string? GroundTruth { get; init; } + [JsonPropertyName("tool_definitions")] public List? ToolDefinitions { get; init; } } diff --git a/dotnet/src/Microsoft.Agents.AI.Foundry/Evaluation/FoundryEvals.cs b/dotnet/src/Microsoft.Agents.AI.Foundry/Evaluation/FoundryEvals.cs index d91b69c1e1..675ae38dfe 100644 --- a/dotnet/src/Microsoft.Agents.AI.Foundry/Evaluation/FoundryEvals.cs +++ b/dotnet/src/Microsoft.Agents.AI.Foundry/Evaluation/FoundryEvals.cs @@ -145,6 +145,8 @@ public sealed class FoundryEvals : IAgentEvaluator bool hasContext = payloads.Any(p => p.Context is not null); bool hasTools = payloads.Any(p => p.ToolDefinitions is { Count: > 0 }); + bool hasGroundTruth = payloads.Any(p => p.GroundTruth is not null); + bool allHaveGroundTruth = payloads.Count > 0 && payloads.All(p => p.GroundTruth is not null); // Filter out tool evaluators if no items have tools; auto-add ToolCallAccuracy if tools present var evaluators = FilterToolEvaluators(this._evaluatorNames, hasTools); @@ -153,13 +155,27 @@ public sealed class FoundryEvals : IAgentEvaluator evaluators = [.. evaluators, ToolCallAccuracy]; } + // Fail fast if a ground-truth evaluator (e.g. similarity) is requested but not + // every item carries an ExpectedOutput. Reference-based evaluators score each + // item against its own ground truth, so even one missing value will surface as + // a provider-side validation error. Catch it here with a clearer message. + var missingGroundTruth = FoundryEvalConverter.FindMissingGroundTruthEvaluators(evaluators, allHaveGroundTruth); + if (missingGroundTruth.Count > 0) + { + throw new InvalidOperationException( + "The following evaluator(s) require a ground-truth/expected output on every item but " + + $"at least one item is missing an {nameof(EvalItem.ExpectedOutput)}: {string.Join(", ", missingGroundTruth)}. " + + "Provide an expected output per item (for example via the 'expectedOutput' parameter on EvaluateAsync), " + + "or set 'includePerAgent: false' so the evaluator only runs on the overall item."); + } + // 2. Create the evaluation definition var createEvalPayload = new WireCreateEvalRequest { Name = evalName, DataSourceConfig = new WireCustomDataSourceConfig { - ItemSchema = FoundryEvalConverter.BuildItemSchema(hasContext, hasTools), + ItemSchema = FoundryEvalConverter.BuildItemSchema(hasContext, hasTools, hasGroundTruth), }, TestingCriteria = FoundryEvalConverter.BuildTestingCriteria( evaluators, this._model, includeDataMapping: true), @@ -822,15 +838,15 @@ public sealed class FoundryEvals : IAgentEvaluator var result = new EvalItemResult(itemId, status, scores); // Extract error info from sample - if (outputItem.TryGetProperty("sample", out var sample)) + if (outputItem.TryGetProperty("sample", out var sample) && sample.ValueKind == JsonValueKind.Object) { - if (sample.TryGetProperty("error", out var errObj)) + if (sample.TryGetProperty("error", out var errObj) && errObj.ValueKind == JsonValueKind.Object) { result.ErrorCode = errObj.TryGetProperty("code", out var code) ? code.GetString() : null; result.ErrorMessage = errObj.TryGetProperty("message", out var msg) ? msg.GetString() : null; } - if (sample.TryGetProperty("usage", out var usage) && usage.TryGetProperty("total_tokens", out var tt) && tt.ValueKind == JsonValueKind.Number) + if (sample.TryGetProperty("usage", out var usage) && usage.ValueKind == JsonValueKind.Object && usage.TryGetProperty("total_tokens", out var tt) && tt.ValueKind == JsonValueKind.Number) { var tokenUsage = new Dictionary(); if (usage.TryGetProperty("prompt_tokens", out var pt) && pt.ValueKind == JsonValueKind.Number) @@ -886,7 +902,7 @@ public sealed class FoundryEvals : IAgentEvaluator } // Extract response_id from datasource_item - if (outputItem.TryGetProperty("datasource_item", out var dsItem)) + if (outputItem.TryGetProperty("datasource_item", out var dsItem) && dsItem.ValueKind == JsonValueKind.Object) { if (dsItem.TryGetProperty("resp_id", out var respId)) { diff --git a/dotnet/src/Microsoft.Agents.AI.Workflows/Evaluation/WorkflowEvaluationExtensions.cs b/dotnet/src/Microsoft.Agents.AI.Workflows/Evaluation/WorkflowEvaluationExtensions.cs index 31cbf08273..223378b787 100644 --- a/dotnet/src/Microsoft.Agents.AI.Workflows/Evaluation/WorkflowEvaluationExtensions.cs +++ b/dotnet/src/Microsoft.Agents.AI.Workflows/Evaluation/WorkflowEvaluationExtensions.cs @@ -28,6 +28,17 @@ public static class WorkflowEvaluationExtensions /// Use , , /// or a custom implementation. /// + /// + /// Optional ground-truth/expected output for the workflow's overall final answer. + /// When provided, it is stamped onto the overall + /// so reference-based evaluators (for example, similarity) can compare the + /// workflow's response against a golden answer. Ground truth is only applied + /// to the overall item; per-agent items are intentionally left without an + /// expected output, since ground truth is defined against the final response. + /// When using a reference-based evaluator that requires ground truth, set + /// to to avoid + /// invoking the evaluator on per-agent items that have no expected output. + /// /// Cancellation token. /// Evaluation results with optional per-agent sub-results. public static async Task EvaluateAsync( @@ -37,6 +48,7 @@ public static class WorkflowEvaluationExtensions bool includePerAgent = true, string evalName = "Workflow Eval", IConversationSplitter? splitter = null, + string? expectedOutput = null, CancellationToken cancellationToken = default) { var events = run.OutgoingEvents.ToList(); @@ -48,28 +60,26 @@ public static class WorkflowEvaluationExtensions var overallItems = new List(); if (includeOverall) { - var finalResponse = events.OfType().LastOrDefault(); - if (finalResponse is not null) + var overallItem = BuildOverallItem(events, splitter, expectedOutput); + if (overallItem is not null) { - var firstInvoked = events.OfType().FirstOrDefault(); - var query = firstInvoked?.Data switch - { - ChatMessage cm => cm.Text ?? string.Empty, - IReadOnlyList msgs => msgs.LastOrDefault(m => m.Role == ChatRole.User)?.Text ?? string.Empty, - string s => s, - _ => firstInvoked?.Data?.ToString() ?? string.Empty, - }; - var conversation = new List - { - new(ChatRole.User, query), - }; - - conversation.AddRange(finalResponse.Response.Messages); - - overallItems.Add(new EvalItem(query, finalResponse.Response.Text, conversation) - { - Splitter = splitter, - }); + overallItems.Add(overallItem); + } + else + { + // The caller asked for an overall evaluation but we couldn't find a final + // response to score — almost always because the workflow's agents weren't + // built with EmitAgentResponseEvents enabled (so no AgentResponseEvent was + // emitted) and no terminal ExecutorCompletedEvent carried an AgentResponse + // / ChatMessage / string payload. Fail loudly instead of silently returning + // 0/0 (or skipping evaluation against a supplied expectedOutput). + throw new InvalidOperationException( + "Cannot evaluate the overall workflow output: no AgentResponseEvent or " + + "ExecutorCompletedEvent with an AgentResponse/ChatMessage/string payload " + + "was found in the run. Bind agents with " + + "AIAgentHostOptions { EmitAgentResponseEvents = true } " + + "(for example via agent.BindAsExecutor(new AIAgentHostOptions { EmitAgentResponseEvents = true })) " + + "so the workflow surfaces the final agent response, or set 'includeOverall: false'."); } } @@ -97,6 +107,86 @@ public static class WorkflowEvaluationExtensions return overallResult; } + internal static EvalItem? BuildOverallItem( + IReadOnlyList events, + IConversationSplitter? splitter, + string? expectedOutput) + { + var firstInvoked = events.OfType().FirstOrDefault(); + var query = firstInvoked?.Data switch + { + ChatMessage cm => cm.Text ?? string.Empty, + IReadOnlyList msgs => msgs.LastOrDefault(m => m.Role == ChatRole.User)?.Text ?? string.Empty, + string s => s, + _ => firstInvoked?.Data?.ToString() ?? string.Empty, + }; + + var conversation = new List + { + new(ChatRole.User, query), + }; + + // Prefer AgentResponseEvent (only emitted when AIAgentHostOptions.EmitAgentResponseEvents + // is enabled). Otherwise fall back to the last ExecutorCompletedEvent that carries an + // AgentResponse / ChatMessage / string payload — these are always emitted by the runtime. + var finalResponse = events.OfType().LastOrDefault(); + string responseText; + if (finalResponse is not null) + { + responseText = finalResponse.Response.Text; + conversation.AddRange(finalResponse.Response.Messages); + } + else + { + ExecutorCompletedEvent? finalCompleted = null; + for (int i = events.Count - 1; i >= 0; i--) + { + if (events[i] is ExecutorCompletedEvent completed + && !IsInternalExecutor(completed.ExecutorId) + && completed.Data is AgentResponse or ChatMessage or string) + { + finalCompleted = completed; + break; + } + } + + if (finalCompleted is null) + { + return null; + } + + switch (finalCompleted.Data) + { + case AgentResponse ar: + responseText = ar.Text; + conversation.AddRange(ar.Messages); + break; + case ChatMessage cm: + responseText = cm.Text ?? string.Empty; + conversation.Add(cm); + break; + case string s: + responseText = s; + conversation.Add(new ChatMessage(ChatRole.Assistant, s)); + break; + default: + // Unreachable — the for-loop above already constrains Data to one of the + // three handled types. Throw if the contract drifts so the bug is visible + // instead of silently dropping the overall item. + throw new InvalidOperationException( + "BuildOverallItem: unexpected ExecutorCompletedEvent.Data type " + + $"'{finalCompleted.Data?.GetType().FullName ?? "null"}'. Expected " + + $"{nameof(AgentResponse)}, {nameof(ChatMessage)}, or string."); + } + } + + return new EvalItem(query, responseText, conversation) + { + Splitter = splitter, + ExpectedOutput = expectedOutput, + }; + } + internal static Dictionary> ExtractAgentData( List events, IConversationSplitter? splitter) diff --git a/dotnet/tests/Microsoft.Agents.AI.Foundry.UnitTests/FoundryEvalConverterTests.cs b/dotnet/tests/Microsoft.Agents.AI.Foundry.UnitTests/FoundryEvalConverterTests.cs index aa0df10200..aea1459e5e 100644 --- a/dotnet/tests/Microsoft.Agents.AI.Foundry.UnitTests/FoundryEvalConverterTests.cs +++ b/dotnet/tests/Microsoft.Agents.AI.Foundry.UnitTests/FoundryEvalConverterTests.cs @@ -179,6 +179,35 @@ public sealed class FoundryEvalConverterTests Assert.Null(payload.Context); } + [Fact] + public void ConvertEvalItem_WithExpectedOutput_PopulatesGroundTruth() + { + // Arrange + var item = new EvalItem(query: "q", response: "r") + { + ExpectedOutput = "the golden answer", + }; + + // Act + var payload = FoundryEvalConverter.ConvertEvalItem(item); + + // Assert + Assert.Equal("the golden answer", payload.GroundTruth); + } + + [Fact] + public void ConvertEvalItem_WithoutExpectedOutput_OmitsGroundTruth() + { + // Arrange + var item = new EvalItem(query: "q", response: "r"); + + // Act + var payload = FoundryEvalConverter.ConvertEvalItem(item); + + // Assert + Assert.Null(payload.GroundTruth); + } + // --------------------------------------------------------------- // FoundryEvalConverter.BuildTestingCriteria tests // --------------------------------------------------------------- @@ -239,6 +268,33 @@ public sealed class FoundryEvalConverterTests Assert.Equal("{{item.context}}", mapping["context"]); } + [Fact] + public void BuildTestingCriteria_SimilarityEvaluator_IncludesGroundTruth() + { + // Act + var criteria = FoundryEvalConverter.BuildTestingCriteria( + ["similarity"], "gpt-4o-mini", includeDataMapping: true); + + // Assert + Assert.Single(criteria); + Assert.Equal("builtin.similarity", criteria[0].EvaluatorName); + var mapping = criteria[0].DataMapping; + Assert.NotNull(mapping); + Assert.True(mapping.ContainsKey("ground_truth")); + Assert.Equal("{{item.ground_truth}}", mapping["ground_truth"]); + } + + [Fact] + public void BuildTestingCriteria_NonGroundTruthEvaluator_OmitsGroundTruth() + { + var criteria = FoundryEvalConverter.BuildTestingCriteria( + ["relevance"], "gpt-4o-mini", includeDataMapping: true); + + var mapping = criteria[0].DataMapping; + Assert.NotNull(mapping); + Assert.False(mapping.ContainsKey("ground_truth")); + } + [Fact] public void BuildTestingCriteria_WithoutDataMapping_OmitsMappingField() { @@ -282,6 +338,59 @@ public sealed class FoundryEvalConverterTests Assert.True(schema.Properties.ContainsKey("tool_definitions")); } + [Fact] + public void BuildItemSchema_WithGroundTruth_IncludesGroundTruthProperty() + { + // Act + var schema = FoundryEvalConverter.BuildItemSchema(hasGroundTruth: true); + + // Assert + Assert.True(schema.Properties.ContainsKey("ground_truth")); + Assert.Equal("string", schema.Properties["ground_truth"].Type); + } + + [Fact] + public void BuildItemSchema_WithoutGroundTruth_OmitsGroundTruthProperty() + { + var schema = FoundryEvalConverter.BuildItemSchema(); + + Assert.False(schema.Properties.ContainsKey("ground_truth")); + } + + // --------------------------------------------------------------- + // FoundryEvalConverter.FindMissingGroundTruthEvaluators tests + // --------------------------------------------------------------- + + [Fact] + public void FindMissingGroundTruthEvaluators_NoGroundTruth_ReturnsSimilarity() + { + // Act + var missing = FoundryEvalConverter.FindMissingGroundTruthEvaluators( + ["similarity", "relevance"], hasGroundTruth: false); + + // Assert + Assert.Single(missing); + Assert.Equal("similarity", missing[0]); + } + + [Fact] + public void FindMissingGroundTruthEvaluators_HasGroundTruth_ReturnsEmpty() + { + var missing = FoundryEvalConverter.FindMissingGroundTruthEvaluators( + ["similarity"], hasGroundTruth: true); + + Assert.Empty(missing); + } + + [Fact] + public void FindMissingGroundTruthEvaluators_NoGroundTruthEvaluators_ReturnsEmpty() + { + var missing = FoundryEvalConverter.FindMissingGroundTruthEvaluators( + ["relevance", "coherence"], hasGroundTruth: false); + + Assert.Empty(missing); + } + // --------------------------------------------------------------- // FoundryEvalConverter.ConvertMessage DataContent test // --------------------------------------------------------------- diff --git a/dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/InputWaiterAndOutputFilterTests.cs b/dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/InputWaiterAndOutputFilterTests.cs index 77c0160200..dead5454b4 100644 --- a/dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/InputWaiterAndOutputFilterTests.cs +++ b/dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/InputWaiterAndOutputFilterTests.cs @@ -36,13 +36,18 @@ public sealed class InputWaiterTests : IDisposable { Task waitTask = this._waiter.WaitForInputAsync(TimeSpan.FromSeconds(5)); - await Task.Delay(50); - waitTask.IsCompleted.Should().BeFalse("the waiter should block until input is signaled"); + Task completedBeforeSignal = await Task.WhenAny(waitTask, Task.Delay(100)); + completedBeforeSignal.Should().NotBeSameAs( + waitTask, + "the waiter should not complete before input is signaled"); this._waiter.SignalInput(); - Task completed = await Task.WhenAny(waitTask, Task.Delay(TimeSpan.FromSeconds(1))); - completed.Should().BeSameAs(waitTask, "the wait task should complete after being signaled"); + Task completedAfterSignal = await Task.WhenAny(waitTask, Task.Delay(TimeSpan.FromSeconds(1))); + completedAfterSignal.Should().BeSameAs( + waitTask, + "the wait task should complete after being signaled"); + await waitTask; } diff --git a/dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/WorkflowEvaluationTests.cs b/dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/WorkflowEvaluationTests.cs index cc4f8338d5..fe7052d440 100644 --- a/dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/WorkflowEvaluationTests.cs +++ b/dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/WorkflowEvaluationTests.cs @@ -1,5 +1,6 @@ // Copyright (c) Microsoft. All rights reserved. +using System; using System.Collections.Generic; using System.Threading.Tasks; using Microsoft.Extensions.AI; @@ -290,6 +291,121 @@ public sealed class WorkflowEvaluationTests Assert.DoesNotContain("end", result.Keys); } + // --------------------------------------------------------------- + // BuildOverallItem tests (expected output / ground truth) + // --------------------------------------------------------------- + + [Fact] + public void BuildOverallItem_NoCompletedExecutorWithResponse_ReturnsNull() + { + // Arrange — no ExecutorCompletedEvent with usable response data and no AgentResponseEvent + var events = new List + { + new ExecutorInvokedEvent("agent-1", "query"), + }; + + // Act + var item = WorkflowEvaluationExtensions.BuildOverallItem(events, splitter: null, expectedOutput: null); + + // Assert + Assert.Null(item); + } + + [Fact] + public void BuildOverallItem_NoAgentResponseEvent_FallsBackToLastExecutorCompleted() + { + // Arrange — only ExecutorCompletedEvent (the default when EmitAgentResponseEvents is false) + var finalResponse = new AgentResponse(new ChatMessage(ChatRole.Assistant, "Paris")); + var events = new List + { + new ExecutorInvokedEvent("researcher", "What is the capital of France?"), + new ExecutorCompletedEvent("researcher", new AgentResponse(new ChatMessage(ChatRole.Assistant, "draft"))), + new ExecutorInvokedEvent("editor", "draft"), + new ExecutorCompletedEvent("editor", finalResponse), + }; + + // Act + var item = WorkflowEvaluationExtensions.BuildOverallItem( + events, splitter: null, expectedOutput: "Paris"); + + // Assert + Assert.NotNull(item); + Assert.Equal("What is the capital of France?", item.Query); + Assert.Equal("Paris", item.Response); + Assert.Equal("Paris", item.ExpectedOutput); + } + + [Fact] + public void BuildOverallItem_WithFinalResponseAndExpectedOutput_StampsExpectedOutput() + { + // Arrange + var finalResponse = new AgentResponse(new ChatMessage(ChatRole.Assistant, "Ofrece 41 planes")); + var events = new List + { + new ExecutorInvokedEvent("agent-1", "How many plans does Netlife offer?"), + new ExecutorCompletedEvent("agent-1", finalResponse), + new AgentResponseEvent("agent-1", finalResponse), + }; + + // Act + var item = WorkflowEvaluationExtensions.BuildOverallItem( + events, splitter: null, expectedOutput: "Ofrece 41 planes"); + + // Assert + Assert.NotNull(item); + Assert.Equal("How many plans does Netlife offer?", item.Query); + Assert.Equal("Ofrece 41 planes", item.Response); + Assert.Equal("Ofrece 41 planes", item.ExpectedOutput); + } + + [Fact] + public void BuildOverallItem_WithFinalResponseAndNoExpectedOutput_LeavesExpectedOutputNull() + { + // Arrange + var finalResponse = new AgentResponse(new ChatMessage(ChatRole.Assistant, "answer")); + var events = new List + { + new ExecutorInvokedEvent("agent-1", "query"), + new ExecutorCompletedEvent("agent-1", finalResponse), + new AgentResponseEvent("agent-1", finalResponse), + }; + + // Act + var item = WorkflowEvaluationExtensions.BuildOverallItem(events, splitter: null, expectedOutput: null); + + // Assert + Assert.NotNull(item); + Assert.Null(item.ExpectedOutput); + } + + [Fact] + public async Task EvaluateAsync_WithIncludeOverallButNoFinalResponse_ThrowsAsync() + { + // Arrange — build a workflow whose AIAgentHostExecutor is NOT bound with + // EmitAgentResponseEvents=true, so no AgentResponseEvent is emitted, and the + // ExecutorCompletedEvent for the host carries null Data. That is the scenario + // where BuildOverallItem returns null. When the caller asks for an overall + // evaluation (includeOverall: true), we should fail fast rather than silently + // returning empty results — regardless of whether expectedOutput was supplied. + var agent = new TestEchoAgent(name: "echo"); + var workflow = AgentWorkflowBuilder.BuildSequential(agent); + var input = new List { new(ChatRole.User, "Hello") }; + + var evaluator = new LocalEvaluator( + FunctionEvaluator.Create("noop", (EvalItem _) => true)); + + await using var run = await InProcessExecution.RunAsync(workflow, input); + + // Act + Assert — throws even without expectedOutput + var ex = await Assert.ThrowsAsync(() => + run.EvaluateAsync( + evaluator, + includeOverall: true, + includePerAgent: false)); + + Assert.Contains("EmitAgentResponseEvents", ex.Message); + } + // --------------------------------------------------------------- // EvaluateAsync integration test // --------------------------------------------------------------- From 574631671d447dd29cb9dea71defb882eecadbfa Mon Sep 17 00:00:00 2001 From: Ben Thomas Date: Wed, 13 May 2026 13:07:50 -0700 Subject: [PATCH 2/7] Update version for release. (#5789) Co-authored-by: alliscode <25218250+alliscode@users.noreply.github.com> --- dotnet/nuget/nuget-package.props | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dotnet/nuget/nuget-package.props b/dotnet/nuget/nuget-package.props index d97f72eea3..cb1713e5e9 100644 --- a/dotnet/nuget/nuget-package.props +++ b/dotnet/nuget/nuget-package.props @@ -1,14 +1,14 @@ - 1.5.0 + 1.6.0 1 - 260507 + 260512 $(VersionPrefix)-rc$(RCNumber) $(VersionPrefix)-$(VersionSuffix).$(DateSuffix).1 $(VersionPrefix)-preview.$(DateSuffix).1 $(VersionPrefix) - 1.5.0 + 1.6.0 Debug;Release;Publish true From 7d23582e2b5a94366ca08fc74562b1a5317f05aa Mon Sep 17 00:00:00 2001 From: Giles Odigwe <79032838+giles17@users.noreply.github.com> Date: Wed, 13 May 2026 13:09:59 -0700 Subject: [PATCH 3/7] Python: fix: prevent MCP message_handler deadlock on notification reload (#4866) * fix(python): prevent MCP message_handler deadlock on notification reload When an MCP server sends a notifications/tools/list_changed or notifications/prompts/list_changed notification, the message_handler previously awaited load_tools()/load_prompts() directly. Since the handler runs on the MCP SDK's single-threaded receive loop, this caused a deadlock: load_tools() sends a list_tools request and waits for its response, but the receive loop cannot deliver that response while blocked in the handler. This manifested as a timeout in call_tool(), which then surfaced as "Error: Function failed." to the model instead of the real tool output. The MATLAB MCP server reliably triggers this because it sends a tools/list_changed notification during tool execution. Fix: schedule reloads as background asyncio.Tasks via a new _schedule_reload() helper, freeing the receive loop immediately. Fixes #4828 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address PR review feedback: fix exc_info, coalesce reloads, shutdown cleanup, tests - Fix exc_info=exc -> exc_info=True in _schedule_reload and message_handler - Tighten _schedule_reload param type from Any to Coroutine[Any, Any, None] - Coalesce reloads: cancel-and-replace per reload kind to prevent unbounded growth - Cancel pending reload tasks in _close_on_owner before tearing down session - Re-raise CancelledError in _safe_reload to respect task cancellation - Replace flaky asyncio.sleep(0) with asyncio.wait_for/gather in tests - Add caplog assertions to verify reload failure is actually logged - Assert _pending_reload_tasks cleanup on error path Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: address review comments on MCP reload handling - Fix exc_info=True -> exc_info=message in message_handler error logging, since the handler is not called from an except block - Await cancelled reload tasks in _close_on_owner before tearing down the session to avoid 'Task was destroyed but pending' warnings - Add cancel-and-replace test verifying duplicate notifications cancel the first reload task and only keep one in flight Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: remove Task.cancelling() call for Python 3.10 compat Task.cancelling() was added in Python 3.11. Replace with awaiting the task and checking cancelled() instead. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Add debug log when cancelling superseded reload task Log at DEBUG level when a new notification cancels an in-flight reload task, improving observability of the cancel-and-replace behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- python/packages/core/agent_framework/_mcp.py | 50 ++++++++- python/packages/core/tests/core/test_mcp.py | 112 ++++++++++++++++++- 2 files changed, 158 insertions(+), 4 deletions(-) diff --git a/python/packages/core/agent_framework/_mcp.py b/python/packages/core/agent_framework/_mcp.py index 0d85b1699a..a7a3f1a796 100644 --- a/python/packages/core/agent_framework/_mcp.py +++ b/python/packages/core/agent_framework/_mcp.py @@ -10,7 +10,7 @@ import logging import re import sys from abc import abstractmethod -from collections.abc import Callable, Collection, Sequence +from collections.abc import Callable, Collection, Coroutine, Sequence from contextlib import AsyncExitStack, _AsyncGeneratorContextManager # type: ignore from datetime import timedelta from functools import partial @@ -264,6 +264,7 @@ class MCPTool: self.is_connected: bool = False self._tools_loaded: bool = False self._prompts_loaded: bool = False + self._pending_reload_tasks: set[asyncio.Task[None]] = set() def __str__(self) -> str: return f"MCPTool(name={self.name}, description={self.description})" @@ -905,12 +906,47 @@ class MCPTool: if isinstance(message, types.ServerNotification): match message.root.method: case "notifications/tools/list_changed": - await self.load_tools() + self._schedule_reload(self.load_tools()) case "notifications/prompts/list_changed": - await self.load_prompts() + self._schedule_reload(self.load_prompts()) case _: logger.debug("Unhandled notification: %s", message.root.method) + def _schedule_reload(self, coro: Coroutine[Any, Any, None]) -> None: + """Schedule a reload coroutine as a background task. + + Reloads (load_tools / load_prompts) triggered by MCP server + notifications must NOT be awaited inside the message handler because + the handler runs on the MCP SDK's single-threaded receive loop. + Awaiting a session request (e.g. ``list_tools``) from within that loop + deadlocks: the receive loop cannot read the response while it is + blocked waiting for the handler to return. + + Instead we fire the reload as an independent ``asyncio.Task`` and keep + a strong reference in ``_pending_reload_tasks`` so it is not garbage- + collected before completion. Only one reload per kind (tools / prompts) + is kept in flight; a new notification cancels the previous pending task + for the same coroutine name to avoid unbounded growth. + """ + # Cancel-and-replace: only one reload per kind should be in flight. + reload_name = f"mcp-reload:{self.name}:{coro.__qualname__}" + for existing in list(self._pending_reload_tasks): + if existing.get_name() == reload_name and not existing.done(): + logger.debug("Cancelling in-flight reload %s; superseded by new notification", reload_name) + existing.cancel() + + async def _safe_reload() -> None: + try: + await coro + except asyncio.CancelledError: + raise + except Exception: + logger.warning("Background MCP reload failed", exc_info=True) + + task = asyncio.create_task(_safe_reload(), name=reload_name) + self._pending_reload_tasks.add(task) + task.add_done_callback(self._pending_reload_tasks.discard) + def _determine_approval_mode( self, *candidate_names: str, @@ -1047,6 +1083,14 @@ class MCPTool: params = types.PaginatedRequestParams(cursor=tool_list.nextCursor) async def _close_on_owner(self) -> None: + # Cancel any pending reload tasks before tearing down the session. + tasks = list(self._pending_reload_tasks) + for task in tasks: + task.cancel() + self._pending_reload_tasks.clear() + if tasks: + await asyncio.gather(*tasks, return_exceptions=True) + await self._safe_close_exit_stack() self._exit_stack = AsyncExitStack() self.session = None diff --git a/python/packages/core/tests/core/test_mcp.py b/python/packages/core/tests/core/test_mcp.py index 487331e3f0..cd3173a7d3 100644 --- a/python/packages/core/tests/core/test_mcp.py +++ b/python/packages/core/tests/core/test_mcp.py @@ -1,6 +1,7 @@ # Copyright (c) Microsoft. All rights reserved. # type: ignore[reportPrivateUsage] import asyncio +import contextlib import json import logging import os @@ -1615,7 +1616,7 @@ async def test_mcp_connection_reset_integration(): async def test_mcp_tool_message_handler_notification(): """Test that message_handler correctly processes tools/list_changed and prompts/list_changed - notifications.""" + notifications by scheduling reloads as background tasks.""" tool = MCPStdioTool(name="test_tool", command="python") # Mock the load_tools and load_prompts methods @@ -1629,6 +1630,8 @@ async def test_mcp_tool_message_handler_notification(): result = await tool.message_handler(tools_notification) assert result is None + # The reload is scheduled as a background task; let it run. + await asyncio.sleep(0) tool.load_tools.assert_called_once() # Reset mock @@ -1641,6 +1644,7 @@ async def test_mcp_tool_message_handler_notification(): result = await tool.message_handler(prompts_notification) assert result is None + await asyncio.sleep(0) tool.load_prompts.assert_called_once() # Test unhandled notification @@ -1664,6 +1668,112 @@ async def test_mcp_tool_message_handler_error(): assert result is None +async def test_mcp_tool_message_handler_does_not_block_receive_loop(): + """Test that message_handler does not deadlock the MCP receive loop. + + Regression test for https://github.com/microsoft/agent-framework/issues/4828. + When the MCP server sends a ``notifications/tools/list_changed`` + notification, the handler must NOT await ``load_tools()`` synchronously + because that would block the single-threaded MCP receive loop, preventing + it from delivering the ``list_tools`` response — a classic deadlock. + """ + tool = MCPStdioTool(name="test_tool", command="python") + + # Use an event to make load_tools block until we release it. + # This simulates load_tools waiting for a session response that the + # receive loop would need to deliver. + release = asyncio.Event() + + async def slow_load_tools(): + await release.wait() + + tool.load_tools = slow_load_tools # type: ignore[assignment] + + tools_notification = Mock(spec=types.ServerNotification) + tools_notification.root = Mock() + tools_notification.root.method = "notifications/tools/list_changed" + + # message_handler must return immediately even though load_tools blocks. + await tool.message_handler(tools_notification) + + # If the handler had awaited load_tools synchronously, we would never + # reach this line (deadlock). Verify the reload task is pending. + assert len(tool._pending_reload_tasks) == 1 + + # Unblock the reload so the background task finishes cleanly. + release.set() + # Wait for the pending reload task(s) to complete so their done-callbacks + # have a chance to remove them from _pending_reload_tasks. + await asyncio.wait_for(asyncio.gather(*tool._pending_reload_tasks), timeout=1) + assert len(tool._pending_reload_tasks) == 0 + + +async def test_mcp_tool_message_handler_reload_failure_is_logged(caplog: pytest.LogCaptureFixture): + """Background reload errors are logged, not raised into the receive loop.""" + tool = MCPStdioTool(name="test_tool", command="python") + tool.load_tools = AsyncMock(side_effect=RuntimeError("connection lost")) + + tools_notification = Mock(spec=types.ServerNotification) + tools_notification.root = Mock() + tools_notification.root.method = "notifications/tools/list_changed" + + await tool.message_handler(tools_notification) + # Let the background task run — it should not propagate the exception. + # Snapshot tasks and await them to ensure done-callbacks fire. + pending = list(tool._pending_reload_tasks) + if pending: + await asyncio.wait_for(asyncio.gather(*pending, return_exceptions=True), timeout=1) + tool.load_tools.assert_called_once() + assert len(tool._pending_reload_tasks) == 0 + + # Verify the warning was actually logged with exception info. + reload_warnings = [r for r in caplog.records if "Background MCP reload failed" in r.message] + assert len(reload_warnings) == 1 + assert reload_warnings[0].levelname == "WARNING" + assert reload_warnings[0].exc_info is not None + + +async def test_mcp_tool_message_handler_cancel_and_replace(): + """Sending two notifications in quick succession cancels the first reload task.""" + tool = MCPStdioTool(name="test_tool", command="python") + + release = asyncio.Event() + call_count = 0 + + async def blocking_load_tools(): + nonlocal call_count + call_count += 1 + await release.wait() + + tool.load_tools = blocking_load_tools # type: ignore[assignment] + + notification = Mock(spec=types.ServerNotification) + notification.root = Mock() + notification.root.method = "notifications/tools/list_changed" + + # First notification — starts a blocking reload task. + await tool.message_handler(notification) + assert len(tool._pending_reload_tasks) == 1 + first_task = next(iter(tool._pending_reload_tasks)) + + # Second notification — should cancel the first and replace it. + await tool.message_handler(notification) + # Yield to the event loop so the cancellation is processed. + with contextlib.suppress(asyncio.CancelledError): + await first_task + + assert first_task.cancelled() + + assert len(tool._pending_reload_tasks) == 1 + second_task = next(iter(tool._pending_reload_tasks)) + assert second_task is not first_task + + # Unblock and let the second task finish. + release.set() + await asyncio.wait_for(asyncio.gather(*tool._pending_reload_tasks), timeout=1) + assert len(tool._pending_reload_tasks) == 0 + + async def test_mcp_tool_sampling_callback_no_client(): """Test sampling callback error path when no chat client is available.""" tool = MCPStdioTool(name="test_tool", command="python") From ab09246dc40a5c7b65ab675b1dc230210d7ea5f4 Mon Sep 17 00:00:00 2001 From: SergeyMenshykh <68852919+SergeyMenshykh@users.noreply.github.com> Date: Wed, 13 May 2026 21:35:52 +0100 Subject: [PATCH 4/7] [Python] [Breaking] Extract skill spec metadata into SkillFrontmatter (#5775) * Fix Skill docstring consistency and spelling - Add ClassSkill to Skill class docstring concrete implementations list - Normalize 'defence' to 'defense' for American English consistency - Remove extra blank line in InlineSkill docstring example Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix E501 line-too-long lint error in test_skills.py Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix stale test section header to reflect SkillFrontmatter API Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix metadata children overriding top-level frontmatter fields Scope YAML_KV_RE to column-0 keys only so indented children under metadata: are not mistakenly parsed as top-level fields. Add regression test and spec fields to sample SKILL.md files. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- python/packages/core/AGENTS.md | 3 +- .../packages/core/agent_framework/__init__.py | 2 + .../packages/core/agent_framework/_skills.py | 341 +++++--- .../packages/core/tests/core/test_skills.py | 802 ++++++++++++------ .../code_defined_skill/code_defined_skill.py | 7 +- .../skills/unit-converter/SKILL.md | 6 + .../skills/mixed_skills/mixed_skills.py | 14 +- .../skills/unit-converter/SKILL.md | 6 + .../skills/script_approval/script_approval.py | 7 +- .../skills/skill_filtering/skill_filtering.py | 2 +- .../skills/length-converter/SKILL.md | 6 + .../skills/volume-converter/SKILL.md | 6 + 12 files changed, 844 insertions(+), 358 deletions(-) diff --git a/python/packages/core/AGENTS.md b/python/packages/core/AGENTS.md index fafbc55f2f..edd4eaa158 100644 --- a/python/packages/core/AGENTS.md +++ b/python/packages/core/AGENTS.md @@ -69,7 +69,8 @@ agent_framework/ ### Skills (`_skills.py`) -- **`Skill`** - A skill definition bundling instructions (`content`) with metadata, resources, and scripts. Supports `@skill.resource` and `@skill.script` decorators for adding components. +- **`Skill`** - Abstract base for a skill definition bundling instructions (`content`) with frontmatter metadata, resources, and scripts. Concrete subclasses (`InlineSkill`, `FileSkill`, `ClassSkill`) accept a `frontmatter=SkillFrontmatter(...)` argument carrying the spec fields. Adding new spec fields is done in one place — on `SkillFrontmatter` — keeping the subclass constructors stable. +- **`SkillFrontmatter`** - L1 discovery metadata for a skill (`name`, `description`, `license`, `compatibility`, `allowed_tools`, `metadata`). All fields are mutable plain attributes; the constructor validates `name`, `description`, and `compatibility` against the spec but post-construction assignments are not re-validated. Spec fields are reachable on every skill via `skill.frontmatter`. - **`SkillResource`** - Named supplementary content attached to a skill; holds either static `content` or a dynamic `function` (sync or async). Exactly one must be provided. - **`SkillScript`** - An executable script attached to a skill; holds either an inline `function` (code-defined, runs in-process) or a `path` to a file on disk (file-based, delegated to a runner). Exactly one must be provided. - **`SkillScriptRunner`** - Protocol for file-based script execution. Any callable matching `(skill, script, args) -> Any` satisfies it. Code-defined scripts do not use a runner. diff --git a/python/packages/core/agent_framework/__init__.py b/python/packages/core/agent_framework/__init__.py index db1c43abfe..356051da3f 100644 --- a/python/packages/core/agent_framework/__init__.py +++ b/python/packages/core/agent_framework/__init__.py @@ -147,6 +147,7 @@ from ._skills import ( InlineSkillScript, InMemorySkillsSource, Skill, + SkillFrontmatter, SkillResource, SkillScript, SkillScriptRunner, @@ -432,6 +433,7 @@ __all__ = [ "SessionContext", "SingleEdgeGroup", "Skill", + "SkillFrontmatter", "SkillResource", "SkillScript", "SkillScriptRunner", diff --git a/python/packages/core/agent_framework/_skills.py b/python/packages/core/agent_framework/_skills.py index 44755a2efd..1128a938ea 100644 --- a/python/packages/core/agent_framework/_skills.py +++ b/python/packages/core/agent_framework/_skills.py @@ -465,41 +465,23 @@ class Skill(ABC): A skill represents a domain-specific capability with instructions, resources, and scripts. Concrete implementations include - :class:`FileSkill` (filesystem-backed) and :class:`InlineSkill` - (code-defined). + :class:`FileSkill` (filesystem-backed), :class:`InlineSkill` + (code-defined), and :class:`ClassSkill` (class-based). - Skill metadata follows the - `Agent Skills specification `_. - - Attributes: - name: Skill name (lowercase letters, numbers, hyphens only). - description: Human-readable description of the skill. + Skill spec metadata (name, description, license, compatibility, + allowed_tools, metadata) is exposed via the :attr:`frontmatter` + property, which returns a :class:`SkillFrontmatter` instance. """ - def __init__( - self, - *, - name: str, - description: str, - ) -> None: - """Initialize a Skill. + @property + @abstractmethod + def frontmatter(self) -> SkillFrontmatter: + """The L1 discovery metadata for this skill. - Validates the skill name and description against specification rules. - - Args: - name: Skill name (lowercase letters, numbers, hyphens only; - max 64 characters; no leading/trailing/consecutive hyphens). - description: Human-readable description of the skill - (≤1024 characters). - - Raises: - ValueError: If the name or description is invalid. + Contains the name, description, and other spec fields as defined by + the `Agent Skills specification `_. """ - _validate_skill_name(name) - _validate_skill_description(name, description) - - self.name = name - self.description = description + ... @property @abstractmethod @@ -535,6 +517,68 @@ class Skill(ABC): return [] +@experimental(feature_id=ExperimentalFeature.SKILLS) +class SkillFrontmatter: + """L1 discovery metadata for a :class:`Skill`. + + Encapsulates all `Agent Skills specification `_ + frontmatter fields in a single object. All fields are mutable plain + attributes; callers may freely reassign them after construction. + + The constructor validates ``name``, ``description``, and ``compatibility`` + against specification rules and raises :class:`ValueError` on invalid + input. Assignments made after construction are **not** re-validated; + callers are expected to honor the spec. + + Attributes: + name: Skill name (lowercase letters, numbers, hyphens only). + description: Human-readable description of the skill. + license: Optional license name or reference. + compatibility: Optional compatibility information (≤500 characters). + allowed_tools: Optional space-delimited pre-approved tool names. + metadata: Optional arbitrary key-value pairs (shallow-copied on + construction to avoid caller-owned dict aliasing). + """ + + def __init__( + self, + *, + name: str, + description: str, + license: str | None = None, + compatibility: str | None = None, + allowed_tools: str | None = None, + metadata: dict[str, str] | None = None, + ) -> None: + """Initialize a SkillFrontmatter. + + Args: + name: Skill name (lowercase letters, numbers, hyphens only; + max 64 characters; no leading/trailing/consecutive hyphens). + description: Human-readable description of the skill + (≤1024 characters). + license: Optional license name or reference. + compatibility: Optional compatibility information + (≤500 characters). + allowed_tools: Optional space-delimited pre-approved tool names. + metadata: Optional arbitrary key-value pairs. + + Raises: + ValueError: If the name, description, or compatibility is invalid. + """ + _validate_skill_name(name) + _validate_skill_description(name, description) + _validate_compatibility(compatibility) + + self.name = name + self.description = description + self.compatibility = compatibility + self.license = license + self.allowed_tools = allowed_tools + # Shallow-copy to avoid aliasing with caller-owned dict. + self.metadata: dict[str, str] | None = dict(metadata) if metadata is not None else None + + def _validate_skill_name(name: str) -> None: """Validate a skill name against specification rules. @@ -573,6 +617,21 @@ def _validate_skill_description(name: str, description: str) -> None: ) +def _validate_compatibility(compatibility: str | None) -> None: + """Validate an optional compatibility value against specification rules. + + Args: + compatibility: The optional compatibility value to validate. + + Raises: + ValueError: If the value exceeds the maximum allowed length. + """ + if compatibility is not None and len(compatibility) > MAX_COMPATIBILITY_LENGTH: + raise ValueError( + f"Skill compatibility must be {MAX_COMPATIBILITY_LENGTH} characters or fewer." + ) + + def _build_skill_content( name: str, description: str, @@ -639,23 +698,17 @@ class InlineSkill(Skill): All resources and scripts should be configured before the skill is registered with a :class:`SkillsProvider`. - Attributes: - name: Skill name (lowercase letters, numbers, hyphens only). - description: Human-readable description of the skill. - instructions: The skill instructions text. - Examples: - With the decorator: - .. code-block:: python skill = InlineSkill( - name="db-skill", - description="Database operations", + frontmatter=SkillFrontmatter( + name="db-skill", + description="Database operations", + ), instructions="Use this skill for DB tasks.", ) - @skill.resource def get_schema() -> str: return "CREATE TABLE ..." @@ -664,8 +717,7 @@ class InlineSkill(Skill): def __init__( self, *, - name: str, - description: str, + frontmatter: SkillFrontmatter, instructions: str, resources: Sequence[SkillResource] | None = None, scripts: Sequence[SkillScript] | None = None, @@ -673,19 +725,25 @@ class InlineSkill(Skill): """Initialize an InlineSkill. Args: - name: Skill name (lowercase letters, numbers, hyphens only). - description: Human-readable description of the skill (≤1024 chars). + frontmatter: Skill specification metadata (name, description, + and optional spec fields). Construct a :class:`SkillFrontmatter` + with the desired fields. instructions: The skill instructions text. resources: Pre-built resources to attach to this skill. scripts: Pre-built scripts to attach to this skill. """ - super().__init__(name=name, description=description) + self._frontmatter = frontmatter self.instructions = instructions self._resources: list[SkillResource] = list(resources) if resources is not None else [] self._scripts: list[SkillScript] = list(scripts) if scripts is not None else [] self._cached_content: str | None = None + @property + def frontmatter(self) -> SkillFrontmatter: + """The L1 discovery metadata for this skill.""" + return self._frontmatter + @property def content(self) -> str: """Synthesized XML content with name, description, instructions, resources, and scripts. @@ -697,7 +755,11 @@ class InlineSkill(Skill): return self._cached_content self._cached_content = _build_skill_content( - self.name, self.description, self.instructions, self._resources, self._scripts + self._frontmatter.name, + self._frontmatter.description, + self.instructions, + self._resources, + self._scripts, ) return self._cached_content @@ -932,10 +994,6 @@ class ClassSkill(Skill, ABC): Class-based skills can be distributed via shared libraries or PyPI packages, making them easy to reuse across projects. - Attributes: - name: Skill name (lowercase letters, numbers, hyphens only). - description: Human-readable description of the skill. - Examples: Decorator-based (recommended): @@ -944,8 +1002,10 @@ class ClassSkill(Skill, ABC): class UnitConverterSkill(ClassSkill): def __init__(self) -> None: super().__init__( - name="unit-converter", - description="Convert between common units.", + frontmatter=SkillFrontmatter( + name="unit-converter", + description="Convert between common units.", + ), ) @property @@ -967,8 +1027,10 @@ class ClassSkill(Skill, ABC): class UnitConverterSkill(ClassSkill): def __init__(self) -> None: super().__init__( - name="unit-converter", - description="Convert between common units.", + frontmatter=SkillFrontmatter( + name="unit-converter", + description="Convert between common units.", + ), ) @property @@ -989,22 +1051,25 @@ class ClassSkill(Skill, ABC): def __init__( self, *, - name: str, - description: str, + frontmatter: SkillFrontmatter, ) -> None: """Initialize a ClassSkill. Args: - name: Skill name (lowercase letters, numbers, hyphens only; - max 64 characters). - description: Human-readable description of the skill - (≤1024 characters). + frontmatter: Skill specification metadata (name, description, + and optional spec fields). Construct a :class:`SkillFrontmatter` + with the desired fields. """ - super().__init__(name=name, description=description) + self._frontmatter = frontmatter self._cached_content: str | None = None self._cached_resources: list[SkillResource] | None = None self._cached_scripts: list[SkillScript] | None = None + @property + def frontmatter(self) -> SkillFrontmatter: + """The L1 discovery metadata for this skill.""" + return self._frontmatter + @staticmethod def resource( func: Callable[..., Any] | None = None, @@ -1152,7 +1217,7 @@ class ClassSkill(Skill, ABC): resource_name = marker.get("name") or _make_method_name(attr_name) if resource_name in seen_names: raise ValueError( - f"Skill '{self.name}' already has a resource named '{resource_name}'. " + f"Skill '{self._frontmatter.name}' already has a resource named '{resource_name}'. " "Ensure each @ClassSkill.resource has a unique name." ) seen_names.add(resource_name) @@ -1212,7 +1277,7 @@ class ClassSkill(Skill, ABC): script_name = marker.get("name") or _make_method_name(attr_name) if script_name in seen_names: raise ValueError( - f"Skill '{self.name}' already has a script named '{script_name}'. " + f"Skill '{self._frontmatter.name}' already has a script named '{script_name}'. " "Ensure each @ClassSkill.script has a unique name." ) seen_names.add(script_name) @@ -1240,7 +1305,11 @@ class ClassSkill(Skill, ABC): return self._cached_content self._cached_content = _build_skill_content( - self.name, self.description, self.instructions, self.resources, self.scripts + self._frontmatter.name, + self._frontmatter.description, + self.instructions, + self.resources, + self.scripts, ) return self._cached_content @@ -1250,16 +1319,13 @@ class FileSkill(Skill): """A :class:`Skill` discovered from a filesystem directory backed by a SKILL.md file. Attributes: - name: Skill name (lowercase letters, numbers, hyphens only). - description: Human-readable description of the skill. path: Absolute path to the directory containing this skill. """ def __init__( self, *, - name: str, - description: str, + frontmatter: SkillFrontmatter, content: str, path: str, resources: Sequence[SkillResource] | None = None, @@ -1268,20 +1334,26 @@ class FileSkill(Skill): """Initialize a FileSkill. Args: - name: Skill name (lowercase letters, numbers, hyphens only). - description: Human-readable description of the skill (≤1024 chars). + frontmatter: Skill specification metadata parsed from the + SKILL.md file's YAML frontmatter (name, description, + and optional spec fields). content: The full raw SKILL.md file content including YAML frontmatter. path: Absolute path to the skill directory on disk. resources: Resources discovered for this skill. scripts: Scripts discovered for this skill. """ - super().__init__(name=name, description=description) + self._frontmatter = frontmatter self._content = content self.path = path self._resources: list[SkillResource] = list(resources) if resources is not None else [] self._scripts: list[SkillScript] = list(scripts) if scripts is not None else [] + @property + def frontmatter(self) -> SkillFrontmatter: + """The L1 discovery metadata for this skill.""" + return self._frontmatter + @property def content(self) -> str: """The skill content provided at construction time.""" @@ -1346,6 +1418,7 @@ SKILL_FILE_NAME: Final[str] = "SKILL.md" MAX_SEARCH_DEPTH: Final[int] = 2 MAX_NAME_LENGTH: Final[int] = 64 MAX_DESCRIPTION_LENGTH: Final[int] = 1024 +MAX_COMPATIBILITY_LENGTH: Final[int] = 500 DEFAULT_RESOURCE_EXTENSIONS: Final[tuple[str, ...]] = ( ".md", ".json", @@ -1366,10 +1439,24 @@ FRONTMATTER_RE = re.compile( re.MULTILINE | re.DOTALL, ) -# Matches YAML "key: value" lines. Group 1 = key, Group 2 = quoted value, -# Group 3 = unquoted value. +# Matches top-level YAML "key: value" lines (unindented). Group 1 = key, +# Group 2 = quoted value, Group 3 = unquoted value. Only matches keys at +# column 0 so that indented children (e.g. under "metadata:") are not +# mistakenly captured as top-level fields. YAML_KV_RE = re.compile( - r"^\s*(\w+)\s*:\s*(?:[\"'](.+?)[\"']|(.+?))\s*$", + r"^([\w-]+)\s*:\s*(?:[\"'](.+?)[\"']|(.+?))\s*$", + re.MULTILINE, +) + +# Matches a YAML "metadata:" block followed by indented key-value pairs. +YAML_METADATA_BLOCK_RE = re.compile( + r"^metadata\s*:\s*$\n((?:[ \t]+\S.*\n?)+)", + re.MULTILINE, +) + +# Matches indented "key: value" lines within a metadata block. +YAML_INDENTED_KV_RE = re.compile( + r"^\s+([\w-]+)\s*:\s*(?:[\"'](.+?)[\"']|(.+?))\s*$", re.MULTILINE, ) @@ -1377,6 +1464,7 @@ YAML_KV_RE = re.compile( # must not start or end with a hyphen, and must not contain consecutive hyphens. VALID_NAME_RE = re.compile(r"^[a-z0-9]([a-z0-9]*-[a-z0-9])*[a-z0-9]*$") + # Default system prompt template for advertising available skills to the model. # Use {skills} as the placeholder for the generated skills XML list. DEFAULT_SKILLS_INSTRUCTION_PROMPT = """\ @@ -1463,7 +1551,7 @@ class SkillsProvider(ContextProvider): FileSkillsSource("./skills", script_runner=my_runner), InMemorySkillsSource([my_code_skill]), ]), - predicate=lambda s: s.name != "internal", + predicate=lambda s: s.frontmatter.name != "internal", ) ) provider = SkillsProvider(source) @@ -1698,10 +1786,10 @@ class SkillsProvider(ContextProvider): lines: list[str] = [] # Sort by name for deterministic output - for skill in sorted(skills, key=lambda s: s.name): + for skill in sorted(skills, key=lambda s: s.frontmatter.name): lines.append(" ") - lines.append(f" {xml_escape(skill.name)}") - lines.append(f" {xml_escape(skill.description)}") + lines.append(f" {xml_escape(skill.frontmatter.name)}") + lines.append(f" {xml_escape(skill.frontmatter.description)}") lines.append(" ") return template.format( @@ -1920,7 +2008,7 @@ class SkillsProvider(ContextProvider): def _find_skill(skills: Sequence[Skill], name: str) -> Skill | None: """Find a skill by name (case-insensitive linear scan).""" name_lower = name.lower() - return next((s for s in skills if s.name.lower() == name_lower), None) + return next((s for s in skills if s.frontmatter.name.lower() == name_lower), None) def _load_skill(self, skills: Sequence[Skill], skill_name: str) -> str: """Return the full content for the named skill. @@ -2179,19 +2267,18 @@ class FileSkillsSource(SkillsSource): if parsed is None: continue - name, description, content = parsed + frontmatter, content = parsed - if name in skills: + if frontmatter.name in skills: logger.warning( "Duplicate skill name '%s': skill from '%s' skipped in favor of existing skill", - name, + frontmatter.name, skill_path, ) continue file_skill = FileSkill( - name=name, - description=description, + frontmatter=frontmatter, content=content, path=skill_path, ) @@ -2208,8 +2295,8 @@ class FileSkillsSource(SkillsSource): FileSkillScript(name=sn, full_path=script_full_path, runner=self._script_runner) ) - skills[file_skill.name] = file_skill - logger.info("Loaded skill: %s", file_skill.name) + skills[file_skill.frontmatter.name] = file_skill + logger.info("Loaded skill: %s", file_skill.frontmatter.name) logger.info("Successfully loaded %d skills", len(skills)) return list(skills.values()) @@ -2438,8 +2525,9 @@ class FileSkillsSource(SkillsSource): name: str | None, description: str | None, source: str, + compatibility: str | None = None, ) -> str | None: - """Validate a skill's name and description against naming rules. + """Validate a skill's name, description, and compatibility against naming rules. Enforces length limits, character-set restrictions, and non-emptiness for both file-based and code-defined skills. @@ -2449,6 +2537,7 @@ class FileSkillsSource(SkillsSource): description: Skill description to validate. source: Human-readable label for diagnostics (e.g. a file path or ``"code skill"``). + compatibility: Optional compatibility value to validate. Returns: A diagnostic error string if validation fails, or ``None`` if valid. @@ -2472,24 +2561,32 @@ class FileSkillsSource(SkillsSource): f"Must be {MAX_DESCRIPTION_LENGTH} characters or fewer." ) + if compatibility is not None and len(compatibility) > MAX_COMPATIBILITY_LENGTH: + return ( + f"Skill '{name}' from '{source}' has an invalid compatibility: " + f"Must be {MAX_COMPATIBILITY_LENGTH} characters or fewer." + ) + return None @staticmethod def _extract_frontmatter( content: str, skill_file_path: str, - ) -> tuple[str, str] | None: + ) -> SkillFrontmatter | None: """Extract and validate YAML frontmatter from a SKILL.md file. - Parses the ``---``-delimited frontmatter block for ``name`` and - ``description`` fields. + Parses the ``---``-delimited frontmatter block for all + `agentskills.io specification `_ + fields: ``name``, ``description``, ``license``, ``compatibility``, + ``allowed-tools``, and ``metadata``. Args: content: Raw text content of the SKILL.md file. skill_file_path: Path to the file (used in diagnostic messages only). Returns: - A ``(name, description)`` tuple on success, or ``None`` if the + A :class:`SkillFrontmatter` on success, or ``None`` if the frontmatter is missing, malformed, or fails validation. """ match = FRONTMATTER_RE.search(content) @@ -2500,35 +2597,63 @@ class FileSkillsSource(SkillsSource): yaml_content = match.group(1).strip() name: str | None = None description: str | None = None + license_value: str | None = None + compatibility: str | None = None + allowed_tools: str | None = None for kv_match in YAML_KV_RE.finditer(yaml_content): key = kv_match.group(1) value = kv_match.group(2) if kv_match.group(2) is not None else kv_match.group(3) - if key.lower() == "name": + key_lower = key.lower() + if key_lower == "name": name = value - elif key.lower() == "description": + elif key_lower == "description": description = value + elif key_lower == "license": + license_value = value + elif key_lower == "compatibility": + compatibility = value + elif key_lower == "allowed-tools": + allowed_tools = value - error = FileSkillsSource._validate_skill_metadata(name, description, skill_file_path) + # Parse metadata block (indented key-value pairs under "metadata:"). + metadata: dict[str, str] | None = None + metadata_match = YAML_METADATA_BLOCK_RE.search(yaml_content) + if metadata_match: + metadata = {} + for kv_match in YAML_INDENTED_KV_RE.finditer(metadata_match.group(1)): + mk = kv_match.group(1) + mv = kv_match.group(2) if kv_match.group(2) is not None else kv_match.group(3) + metadata[mk] = mv + + error = FileSkillsSource._validate_skill_metadata(name, description, skill_file_path, compatibility) if error: logger.error(error) return None - # name and description are guaranteed non-None after validation - return name, description # type: ignore[return-value] + # name and description are guaranteed non-None after validation; + # SkillFrontmatter re-validates as a defense-in-depth invariant. + return SkillFrontmatter( + name=cast(str, name), + description=cast(str, description), + license=license_value, + compatibility=compatibility, + allowed_tools=allowed_tools, + metadata=metadata, + ) @staticmethod def _read_and_parse_skill_file( skill_dir_path: str, - ) -> tuple[str, str, str] | None: + ) -> tuple[SkillFrontmatter, str] | None: """Read and parse the SKILL.md file in *skill_dir_path*. Args: skill_dir_path: Absolute path to the directory containing ``SKILL.md``. Returns: - A ``(name, description, content)`` tuple where *content* is the + A ``(frontmatter, content)`` tuple where *content* is the full raw file text, or ``None`` if the file cannot be read or its frontmatter is invalid. """ @@ -2540,23 +2665,21 @@ class FileSkillsSource(SkillsSource): logger.error("Failed to read SKILL.md at '%s'", skill_file) return None - result = FileSkillsSource._extract_frontmatter(content, str(skill_file)) - if result is None: + frontmatter = FileSkillsSource._extract_frontmatter(content, str(skill_file)) + if frontmatter is None: return None - name, description = result - dir_name = Path(skill_dir_path).name - if name != dir_name: + if frontmatter.name != dir_name: logger.error( "SKILL.md at '%s' has frontmatter name '%s' that does not match the directory name '%s'; skipping.", skill_file, - name, + frontmatter.name, dir_name, ) return None - return name, description, content + return frontmatter, content @staticmethod def _discover_skill_directories(skill_paths: Sequence[str]) -> list[str]: @@ -2704,12 +2827,12 @@ class DeduplicatingSkillsSource(DelegatingSkillsSource): result: list[Skill] = [] for skill in skills: - key = skill.name.lower() + key = skill.frontmatter.name.lower() if key in seen: logger.warning( "Duplicate skill name '%s': skill skipped in favor of existing skill '%s'", - skill.name, - seen[key].name, + skill.frontmatter.name, + seen[key].frontmatter.name, ) continue seen[key] = skill @@ -2730,7 +2853,7 @@ class FilteringSkillsSource(DelegatingSkillsSource): filtered = FilteringSkillsSource( inner_source=my_source, - predicate=lambda s: s.name != "internal", + predicate=lambda s: s.frontmatter.name != "internal", ) skills = await filtered.get_skills() """ diff --git a/python/packages/core/tests/core/test_skills.py b/python/packages/core/tests/core/test_skills.py index b268b31551..de39c58b2f 100644 --- a/python/packages/core/tests/core/test_skills.py +++ b/python/packages/core/tests/core/test_skills.py @@ -24,6 +24,7 @@ from agent_framework import ( InMemorySkillsSource, SessionContext, Skill, + SkillFrontmatter, SkillResource, SkillScript, SkillScriptRunner, @@ -69,7 +70,7 @@ def _ctx(provider: SkillsProvider) -> tuple[dict[str, Skill], str | None, list[A ctx = provider._cached_context # pyright: ignore[reportPrivateUsage] assert ctx is not None, "_init_provider() must be called before accessing context" skills, instructions, tools = ctx - return {s.name: s for s in skills}, instructions, tools + return {s.frontmatter.name: s for s in skills}, instructions, tools def _raw_skills(provider: SkillsProvider) -> Sequence[Skill]: @@ -129,10 +130,9 @@ def _read_and_parse_skill_file_for_test(skill_dir: Path) -> FileSkill: """Parse a SKILL.md file from the given directory, raising if invalid.""" result = FileSkillsSource._read_and_parse_skill_file(str(skill_dir)) assert result is not None, f"Failed to parse skill at {skill_dir}" - name, description, content = result + frontmatter, content = result return FileSkill( - name=name, - description=description, + frontmatter=frontmatter, content=content, path=str(skill_dir), ) @@ -163,7 +163,7 @@ async def _discover_file_skills_for_test( result: dict[str, FileSkill] = {} for s in skills: assert isinstance(s, FileSkill), f"Expected FileSkill, got {type(s).__name__}" - result[s.name] = s + result[s.frontmatter.name] = s return result @@ -268,22 +268,21 @@ class TestTryParseSkillDocument: content = "---\nname: test-skill\ndescription: A test skill.\n---\n# Body\nInstructions here." result = FileSkillsSource._extract_frontmatter(content, "test.md") assert result is not None - name, description = result - assert name == "test-skill" - assert description == "A test skill." + assert result.name == "test-skill" + assert result.description == "A test skill." def test_quoted_values(self) -> None: content = "---\nname: \"test-skill\"\ndescription: 'A test skill.'\n---\nBody." result = FileSkillsSource._extract_frontmatter(content, "test.md") assert result is not None - assert result[0] == "test-skill" - assert result[1] == "A test skill." + assert result.name == "test-skill" + assert result.description == "A test skill." def test_utf8_bom(self) -> None: content = "\ufeff---\nname: test-skill\ndescription: A test skill.\n---\nBody." result = FileSkillsSource._extract_frontmatter(content, "test.md") assert result is not None - assert result[0] == "test-skill" + assert result.name == "test-skill" def test_missing_frontmatter(self) -> None: content = "# Just a markdown file\nNo frontmatter here." @@ -327,11 +326,11 @@ class TestTryParseSkillDocument: result = FileSkillsSource._extract_frontmatter(content, "test.md") assert result is None - def test_extra_metadata_ignored(self) -> None: + def test_extra_fields_parsed(self) -> None: content = "---\nname: test-skill\ndescription: A test skill.\nauthor: someone\nversion: 1.0\n---\nBody." result = FileSkillsSource._extract_frontmatter(content, "test.md") assert result is not None - assert result[0] == "test-skill" + assert result.name == "test-skill" # --------------------------------------------------------------------------- @@ -346,7 +345,7 @@ class TestDiscoverAndLoadSkills: _write_skill(tmp_path, "my-skill") skills = await _discover_file_skills_for_test([str(tmp_path)]) assert "my-skill" in skills - assert skills["my-skill"].name == "my-skill" + assert skills["my-skill"].frontmatter.name == "my-skill" async def test_discovers_nested_skills(self, tmp_path: Path) -> None: skills_dir = tmp_path / "skills" @@ -504,7 +503,7 @@ class TestBuildSkillsInstructionPrompt: def test_default_prompt_contains_skills(self) -> None: skills = [ - InlineSkill(name="my-skill", description="Does stuff.", instructions="Body"), + InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="Does stuff."), instructions="Body"), ] prompt = SkillsProvider._create_instructions(None, skills) assert prompt is not None @@ -514,8 +513,8 @@ class TestBuildSkillsInstructionPrompt: def test_skills_sorted_alphabetically(self) -> None: skills = [ - InlineSkill(name="zebra", description="Z skill.", instructions="Body"), - InlineSkill(name="alpha", description="A skill.", instructions="Body"), + InlineSkill(frontmatter=SkillFrontmatter(name="zebra", description="Z skill."), instructions="Body"), + InlineSkill(frontmatter=SkillFrontmatter(name="alpha", description="A skill."), instructions="Body"), ] prompt = SkillsProvider._create_instructions(None, skills) assert prompt is not None @@ -525,7 +524,9 @@ class TestBuildSkillsInstructionPrompt: def test_xml_escapes_metadata(self) -> None: skills = [ - InlineSkill(name="my-skill", description='Uses & "quotes"', instructions="Body"), + InlineSkill( + frontmatter=SkillFrontmatter(name="my-skill", description='Uses & "quotes"'), instructions="Body" + ), ] prompt = SkillsProvider._create_instructions(None, skills) assert prompt is not None @@ -534,7 +535,7 @@ class TestBuildSkillsInstructionPrompt: def test_custom_prompt_template(self) -> None: skills = [ - InlineSkill(name="my-skill", description="Does stuff.", instructions="Body"), + InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="Does stuff."), instructions="Body"), ] custom = "Custom header:\n{skills}\nCustom footer." prompt = SkillsProvider._create_instructions(custom, skills) @@ -544,14 +545,14 @@ class TestBuildSkillsInstructionPrompt: def test_invalid_prompt_template_raises(self) -> None: skills = [ - InlineSkill(name="my-skill", description="Does stuff.", instructions="Body"), + InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="Does stuff."), instructions="Body"), ] with pytest.raises(ValueError, match="valid format string"): SkillsProvider._create_instructions("{invalid}", skills) def test_positional_placeholder_raises(self) -> None: skills = [ - InlineSkill(name="my-skill", description="Does stuff.", instructions="Body"), + InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="Does stuff."), instructions="Body"), ] with pytest.raises(ValueError, match="valid format string"): SkillsProvider._create_instructions("Header {0} footer", skills) @@ -942,25 +943,28 @@ class TestInlineSkill: def test_inline_skill_is_skill(self) -> None: """InlineSkill is a subclass of Skill.""" - skill = InlineSkill(name="my-skill", description="A skill.", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), instructions="Body") assert isinstance(skill, Skill) def test_file_skill_is_skill(self) -> None: """FileSkill is a subclass of Skill.""" - skill = FileSkill(name="my-skill", description="A skill.", content="Body", path="/tmp/skill") + skill = FileSkill( + frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), content="Body", path="/tmp/skill" + ) assert isinstance(skill, Skill) def test_basic_construction(self) -> None: - skill = InlineSkill(name="my-skill", description="A test skill.", instructions="Instructions.") - assert skill.name == "my-skill" - assert skill.description == "A test skill." + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="my-skill", description="A test skill."), instructions="Instructions." + ) + assert skill.frontmatter.name == "my-skill" + assert skill.frontmatter.description == "A test skill." assert skill.instructions == "Instructions." assert skill.resources == [] def test_construction_with_static_resources(self) -> None: skill = InlineSkill( - name="my-skill", - description="A test skill.", + frontmatter=SkillFrontmatter(name="my-skill", description="A test skill."), instructions="Instructions.", resources=[ InlineSkillResource(name="ref", content="Reference content"), @@ -971,34 +975,36 @@ class TestInlineSkill: def test_empty_name_raises(self) -> None: with pytest.raises(ValueError, match="cannot be empty"): - InlineSkill(name="", description="A skill.", instructions="Body") + InlineSkill(frontmatter=SkillFrontmatter(name="", description="A skill."), instructions="Body") def test_invalid_name_raises(self) -> None: with pytest.raises(ValueError, match="Invalid skill name"): - InlineSkill(name="Invalid-Name", description="A skill.", instructions="Body") + InlineSkill(frontmatter=SkillFrontmatter(name="Invalid-Name", description="A skill."), instructions="Body") def test_name_starts_with_hyphen_raises(self) -> None: with pytest.raises(ValueError, match="Invalid skill name"): - InlineSkill(name="-bad-name", description="A skill.", instructions="Body") + InlineSkill(frontmatter=SkillFrontmatter(name="-bad-name", description="A skill."), instructions="Body") def test_name_with_consecutive_hyphens_raises(self) -> None: with pytest.raises(ValueError, match="Invalid skill name"): - InlineSkill(name="consecutive--hyphens", description="A skill.", instructions="Body") + InlineSkill( + frontmatter=SkillFrontmatter(name="consecutive--hyphens", description="A skill."), instructions="Body" + ) def test_name_too_long_raises(self) -> None: with pytest.raises(ValueError, match="Invalid skill name"): - InlineSkill(name="a" * 65, description="A skill.", instructions="Body") + InlineSkill(frontmatter=SkillFrontmatter(name="a" * 65, description="A skill."), instructions="Body") def test_empty_description_raises(self) -> None: with pytest.raises(ValueError, match="cannot be empty"): - InlineSkill(name="my-skill", description="", instructions="Body") + InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description=""), instructions="Body") def test_description_too_long_raises(self) -> None: with pytest.raises(ValueError, match="invalid description"): - InlineSkill(name="my-skill", description="a" * 1025, instructions="Body") + InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="a" * 1025), instructions="Body") def test_resource_decorator_bare(self) -> None: - skill = InlineSkill(name="my-skill", description="A skill.", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), instructions="Body") @skill.resource def get_schema() -> Any: @@ -1012,7 +1018,7 @@ class TestInlineSkill: assert skill.resources[0].function is get_schema def test_resource_decorator_with_args(self) -> None: - skill = InlineSkill(name="my-skill", description="A skill.", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), instructions="Body") @skill.resource(name="custom-name", description="Custom description") def my_resource() -> Any: @@ -1024,7 +1030,7 @@ class TestInlineSkill: def test_resource_decorator_returns_function(self) -> None: """Decorator should return the original function unchanged.""" - skill = InlineSkill(name="my-skill", description="A skill.", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), instructions="Body") @skill.resource def get_data() -> Any: @@ -1034,7 +1040,7 @@ class TestInlineSkill: assert get_data() == "data" def test_multiple_resources(self) -> None: - skill = InlineSkill(name="my-skill", description="A skill.", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), instructions="Body") @skill.resource def resource_a() -> Any: @@ -1050,7 +1056,7 @@ class TestInlineSkill: assert "resource_b" in names def test_resource_decorator_async(self) -> None: - skill = InlineSkill(name="my-skill", description="A skill.", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), instructions="Body") @skill.resource async def get_async_data() -> Any: @@ -1070,13 +1076,19 @@ class TestSkillsProviderCodeSkill: """Tests for SkillsProvider with code-defined skills.""" async def test_code_skill_only(self) -> None: - skill = InlineSkill(name="prog-skill", description="A code-defined skill.", instructions="Do the thing.") + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="A code-defined skill."), + instructions="Do the thing.", + ) provider = SkillsProvider([skill]) await _init_provider(provider) assert "prog-skill" in _ctx(provider)[0] async def test_load_skill_returns_content(self) -> None: - skill = InlineSkill(name="prog-skill", description="A skill.", instructions="Code-defined instructions.") + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), + instructions="Code-defined instructions.", + ) provider = SkillsProvider([skill]) await _init_provider(provider) result = provider._load_skill(_raw_skills(provider), "prog-skill") @@ -1087,8 +1099,7 @@ class TestSkillsProviderCodeSkill: async def test_load_skill_appends_resource_listing(self) -> None: skill = InlineSkill( - name="prog-skill", - description="A skill.", + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), instructions="Do things.", resources=[ InlineSkillResource(name="ref-a", content="a", description="First resource"), @@ -1106,7 +1117,9 @@ class TestSkillsProviderCodeSkill: assert '' in result async def test_load_skill_no_resources_no_listing(self) -> None: - skill = InlineSkill(name="prog-skill", description="A skill.", instructions="Body only.") + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), instructions="Body only." + ) provider = SkillsProvider([skill]) await _init_provider(provider) result = provider._load_skill(_raw_skills(provider), "prog-skill") @@ -1115,8 +1128,7 @@ class TestSkillsProviderCodeSkill: async def test_read_static_resource(self) -> None: skill = InlineSkill( - name="prog-skill", - description="A skill.", + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), instructions="Body", resources=[InlineSkillResource(name="ref", content="static content")], ) @@ -1126,7 +1138,9 @@ class TestSkillsProviderCodeSkill: assert result == "static content" async def test_read_callable_resource_sync(self) -> None: - skill = InlineSkill(name="prog-skill", description="A skill.", instructions="Body") + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), instructions="Body" + ) @skill.resource def get_schema() -> Any: @@ -1138,7 +1152,9 @@ class TestSkillsProviderCodeSkill: assert result == "CREATE TABLE users" async def test_read_callable_resource_async(self) -> None: - skill = InlineSkill(name="prog-skill", description="A skill.", instructions="Body") + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), instructions="Body" + ) @skill.resource async def get_data() -> Any: @@ -1151,8 +1167,7 @@ class TestSkillsProviderCodeSkill: async def test_read_resource_case_insensitive(self) -> None: skill = InlineSkill( - name="prog-skill", - description="A skill.", + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), instructions="Body", resources=[InlineSkillResource(name="MyRef", content="content")], ) @@ -1162,14 +1177,18 @@ class TestSkillsProviderCodeSkill: assert result == "content" async def test_read_unknown_resource_returns_error(self) -> None: - skill = InlineSkill(name="prog-skill", description="A skill.", instructions="Body") + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), instructions="Body" + ) provider = SkillsProvider([skill]) await _init_provider(provider) result = await provider._read_skill_resource(_raw_skills(provider), "prog-skill", "nonexistent") assert result.startswith("Error:") async def test_read_callable_resource_sync_with_kwargs(self) -> None: - skill = InlineSkill(name="prog-skill", description="A skill.", instructions="Body") + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), instructions="Body" + ) @skill.resource def get_user_config(**kwargs: Any) -> Any: @@ -1184,7 +1203,9 @@ class TestSkillsProviderCodeSkill: assert result == "config for user_123" async def test_read_callable_resource_async_with_kwargs(self) -> None: - skill = InlineSkill(name="prog-skill", description="A skill.", instructions="Body") + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), instructions="Body" + ) @skill.resource async def get_user_data(**kwargs: Any) -> Any: @@ -1200,7 +1221,9 @@ class TestSkillsProviderCodeSkill: async def test_read_callable_resource_without_kwargs_ignores_extra_args(self) -> None: """Resource functions without **kwargs should still work when kwargs are passed.""" - skill = InlineSkill(name="prog-skill", description="A skill.", instructions="Body") + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), instructions="Body" + ) @skill.resource def static_resource() -> Any: @@ -1215,7 +1238,9 @@ class TestSkillsProviderCodeSkill: async def test_read_callable_resource_returns_dict(self) -> None: """Resource functions may return non-string types, passed through as-is.""" - skill = InlineSkill(name="prog-skill", description="A skill.", instructions="Body") + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), instructions="Body" + ) @skill.resource def get_config() -> Any: @@ -1228,7 +1253,9 @@ class TestSkillsProviderCodeSkill: async def test_read_callable_resource_returns_list(self) -> None: """Resource functions may return lists, passed through as-is.""" - skill = InlineSkill(name="prog-skill", description="A skill.", instructions="Body") + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), instructions="Body" + ) @skill.resource def get_items() -> Any: @@ -1241,7 +1268,9 @@ class TestSkillsProviderCodeSkill: async def test_read_callable_resource_returns_none(self) -> None: """Resource functions may return None.""" - skill = InlineSkill(name="prog-skill", description="A skill.", instructions="Body") + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), instructions="Body" + ) @skill.resource def get_nothing() -> Any: @@ -1253,7 +1282,9 @@ class TestSkillsProviderCodeSkill: assert result is None async def test_before_run_injects_code_skills(self) -> None: - skill = InlineSkill(name="prog-skill", description="A code-defined skill.", instructions="Body") + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="A code-defined skill."), instructions="Body" + ) provider = SkillsProvider([skill]) context = SessionContext(input_messages=[]) @@ -1274,7 +1305,9 @@ class TestSkillsProviderCodeSkill: async def test_combined_file_and_code_skill(self, tmp_path: Path) -> None: _write_skill(tmp_path, "file-skill") - prog_skill = InlineSkill(name="prog-skill", description="Code-defined.", instructions="Body") + prog_skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="Code-defined."), instructions="Body" + ) provider = SkillsProvider( DeduplicatingSkillsSource( AggregatingSkillsSource([ @@ -1289,7 +1322,9 @@ class TestSkillsProviderCodeSkill: async def test_duplicate_name_file_wins(self, tmp_path: Path) -> None: _write_skill(tmp_path, "my-skill", body="File version") - prog_skill = InlineSkill(name="my-skill", description="Code-defined.", instructions="Prog version") + prog_skill = InlineSkill( + frontmatter=SkillFrontmatter(name="my-skill", description="Code-defined."), instructions="Prog version" + ) provider = SkillsProvider( DeduplicatingSkillsSource( AggregatingSkillsSource([ @@ -1304,7 +1339,9 @@ class TestSkillsProviderCodeSkill: async def test_combined_prompt_includes_both(self, tmp_path: Path) -> None: _write_skill(tmp_path, "file-skill") - prog_skill = InlineSkill(name="prog-skill", description="A code-defined skill.", instructions="Body") + prog_skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="A code-defined skill."), instructions="Body" + ) provider = SkillsProvider( DeduplicatingSkillsSource( AggregatingSkillsSource([ @@ -1361,8 +1398,8 @@ class TestFileBasedSkillParsing: def test_name_and_description_from_frontmatter(self, tmp_path: Path) -> None: _write_skill(tmp_path, "my-skill", description="Skill desc.") skill = _read_and_parse_skill_file_for_test(tmp_path / "my-skill") - assert skill.name == "my-skill" - assert skill.description == "Skill desc." + assert skill.frontmatter.name == "my-skill" + assert skill.frontmatter.description == "Skill desc." def test_path_set(self, tmp_path: Path) -> None: _write_skill(tmp_path, "my-skill") @@ -1397,7 +1434,9 @@ class TestLoadSkillFormatting: async def test_code_skill_wraps_in_xml(self) -> None: """Code-defined skills are wrapped with name, description, and instructions tags.""" - skill = InlineSkill(name="prog-skill", description="A skill.", instructions="Do stuff.") + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), instructions="Do stuff." + ) provider = SkillsProvider([skill]) await _init_provider(provider) result = provider._load_skill(_raw_skills(provider), "prog-skill") @@ -1408,8 +1447,7 @@ class TestLoadSkillFormatting: async def test_code_skill_single_resource_no_description(self) -> None: """Resource without description omits the description attribute.""" skill = InlineSkill( - name="prog-skill", - description="A skill.", + frontmatter=SkillFrontmatter(name="prog-skill", description="A skill."), instructions="Body.", resources=[InlineSkillResource(name="data", content="val")], ) @@ -1642,9 +1680,9 @@ class TestReadAndParseSkillFile: (skill_dir / "SKILL.md").write_text("---\nname: my-skill\ndescription: A skill.\n---\nBody.", encoding="utf-8") result = FileSkillsSource._read_and_parse_skill_file(str(skill_dir)) assert result is not None - name, desc, content = result - assert name == "my-skill" - assert desc == "A skill." + frontmatter, content = result + assert frontmatter.name == "my-skill" + assert frontmatter.description == "A skill." assert "Body." in content def test_missing_skill_md_returns_none(self, tmp_path: Path) -> None: @@ -1838,14 +1876,282 @@ class TestExtractFrontmatterEdgeCases: content = f"---\nname: {name}\ndescription: A skill.\n---\nBody." result = FileSkillsSource._extract_frontmatter(content, "test.md") assert result is not None - assert result[0] == name + assert result.name == name def test_description_exactly_max_length(self) -> None: desc = "a" * 1024 content = f"---\nname: test-skill\ndescription: {desc}\n---\nBody." result = FileSkillsSource._extract_frontmatter(content, "test.md") assert result is not None - assert result[1] == desc + assert result.description == desc + + +# --------------------------------------------------------------------------- +# Tests: Skill spec fields (via SkillFrontmatter) +# --------------------------------------------------------------------------- + + +class TestSkillSpecFields: + """Tests for agentskills.io spec fields on SkillFrontmatter exposed via Skill.frontmatter.""" + + def test_basic_construction_defaults(self) -> None: + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="my-skill", description="A description."), instructions="Do it." + ) + assert skill.frontmatter.name == "my-skill" + assert skill.frontmatter.description == "A description." + assert skill.frontmatter.license is None + assert skill.frontmatter.compatibility is None + assert skill.frontmatter.allowed_tools is None + assert skill.frontmatter.metadata is None + + def test_all_fields_on_inline_skill(self) -> None: + skill = InlineSkill( + frontmatter=SkillFrontmatter( + name="my-skill", + description="A description.", + license="MIT", + compatibility="Works with GPT-4", + allowed_tools="tool1 tool2", + metadata={"author": "test", "version": "1.0"}, + ), + instructions="Do it.", + ) + assert skill.frontmatter.license == "MIT" + assert skill.frontmatter.compatibility == "Works with GPT-4" + assert skill.frontmatter.allowed_tools == "tool1 tool2" + assert skill.frontmatter.metadata == {"author": "test", "version": "1.0"} + + def test_compatibility_too_long_raises(self) -> None: + with pytest.raises(ValueError): + InlineSkill( + frontmatter=SkillFrontmatter(name="my-skill", description="A description.", compatibility="a" * 501), + instructions="Do it.", + ) + + def test_compatibility_exactly_max_length(self) -> None: + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="my-skill", description="A description.", compatibility="a" * 500), + instructions="Do it.", + ) + assert skill.frontmatter.compatibility == "a" * 500 + + def test_file_skill_spec_fields(self) -> None: + skill = FileSkill( + frontmatter=SkillFrontmatter( + name="my-skill", + description="Test.", + license="MIT", + compatibility="compat info", + allowed_tools="tool1", + metadata={"key": "val"}, + ), + content="---\nname: my-skill\n---", + path="/skills/my-skill", + ) + assert skill.frontmatter.license == "MIT" + assert skill.frontmatter.compatibility == "compat info" + assert skill.frontmatter.allowed_tools == "tool1" + assert skill.frontmatter.metadata == {"key": "val"} + + +# --------------------------------------------------------------------------- +# Tests: SkillFrontmatter class and two-form constructors +# --------------------------------------------------------------------------- + + +class TestSkillFrontmatter: + """Tests for the :class:`SkillFrontmatter` class.""" + + def test_basic_construction(self) -> None: + fm = SkillFrontmatter(name="my-skill", description="A test skill.") + assert fm.name == "my-skill" + assert fm.description == "A test skill." + assert fm.license is None + assert fm.compatibility is None + assert fm.allowed_tools is None + assert fm.metadata is None + + def test_all_fields(self) -> None: + fm = SkillFrontmatter( + name="my-skill", + description="Desc.", + license="MIT", + compatibility="GPT-4", + allowed_tools="tool1", + metadata={"key": "val"}, + ) + assert fm.license == "MIT" + assert fm.compatibility == "GPT-4" + assert fm.allowed_tools == "tool1" + assert fm.metadata == {"key": "val"} + + def test_invalid_name_raises(self) -> None: + with pytest.raises(ValueError): + SkillFrontmatter(name="Bad Name!", description="Desc.") + + def test_invalid_description_raises(self) -> None: + with pytest.raises(ValueError): + SkillFrontmatter(name="my-skill", description="") + + def test_invalid_compatibility_raises(self) -> None: + with pytest.raises(ValueError): + SkillFrontmatter(name="my-skill", description="Desc.", compatibility="a" * 501) + + def test_compatibility_can_be_reassigned(self) -> None: + fm = SkillFrontmatter(name="my-skill", description="Desc.") + fm.compatibility = "a" * 500 + assert fm.compatibility == "a" * 500 + # Plain attribute: post-construction assignment is not re-validated. + fm.compatibility = "a" * 501 + assert fm.compatibility == "a" * 501 + + def test_metadata_is_shallow_copied(self) -> None: + original = {"key": "val"} + fm = SkillFrontmatter(name="my-skill", description="Desc.", metadata=original) + original["key"] = "mutated" + assert fm.metadata == {"key": "val"} + + def test_name_is_mutable(self) -> None: + fm = SkillFrontmatter(name="my-skill", description="Desc.") + fm.name = "other-skill" + assert fm.name == "other-skill" + + def test_description_is_mutable(self) -> None: + fm = SkillFrontmatter(name="my-skill", description="Desc.") + fm.description = "Other description." + assert fm.description == "Other description." + + +class TestExtractFrontmatterSpecFields: + """Tests for _extract_frontmatter parsing all agentskills.io spec fields.""" + + def test_license_parsed(self) -> None: + content = "---\nname: test-skill\ndescription: A skill.\nlicense: MIT\n---\nBody." + result = FileSkillsSource._extract_frontmatter(content, "test.md") + assert result is not None + assert result.license == "MIT" + + def test_compatibility_parsed(self) -> None: + content = "---\nname: test-skill\ndescription: A skill.\ncompatibility: Works with GPT-4\n---\nBody." + result = FileSkillsSource._extract_frontmatter(content, "test.md") + assert result is not None + assert result.compatibility == "Works with GPT-4" + + def test_compatibility_too_long_returns_none(self) -> None: + long_compat = "a" * 501 + content = f"---\nname: test-skill\ndescription: A skill.\ncompatibility: {long_compat}\n---\nBody." + result = FileSkillsSource._extract_frontmatter(content, "test.md") + assert result is None + + def test_allowed_tools_parsed(self) -> None: + content = "---\nname: test-skill\ndescription: A skill.\nallowed-tools: tool1 tool2 tool3\n---\nBody." + result = FileSkillsSource._extract_frontmatter(content, "test.md") + assert result is not None + assert result.allowed_tools == "tool1 tool2 tool3" + + def test_metadata_block_parsed(self) -> None: + content = ( + "---\nname: test-skill\ndescription: A skill.\nmetadata:\n author: someone\n version: 1.0\n---\nBody." + ) + result = FileSkillsSource._extract_frontmatter(content, "test.md") + assert result is not None + assert result.metadata is not None + assert result.metadata["author"] == "someone" + assert result.metadata["version"] == "1.0" + + def test_metadata_with_quoted_values(self) -> None: + content = ( + "---\nname: test-skill\ndescription: A skill.\nmetadata:\n" + " author: 'John Doe'\n org: \"Contoso\"\n---\nBody." + ) + result = FileSkillsSource._extract_frontmatter(content, "test.md") + assert result is not None + assert result.metadata is not None + assert result.metadata["author"] == "John Doe" + assert result.metadata["org"] == "Contoso" + + def test_no_metadata_block(self) -> None: + content = "---\nname: test-skill\ndescription: A skill.\n---\nBody." + result = FileSkillsSource._extract_frontmatter(content, "test.md") + assert result is not None + assert result.metadata is None + + def test_all_spec_fields(self) -> None: + content = ( + "---\n" + "name: test-skill\n" + "description: A comprehensive skill.\n" + "license: Apache-2.0\n" + "compatibility: Works with GPT-4 and Claude\n" + "allowed-tools: read-file write-file\n" + "metadata:\n" + " author: test-author\n" + " version: 2.0\n" + "---\n" + "Body content." + ) + result = FileSkillsSource._extract_frontmatter(content, "test.md") + assert result is not None + assert result.name == "test-skill" + assert result.description == "A comprehensive skill." + assert result.license == "Apache-2.0" + assert result.compatibility == "Works with GPT-4 and Claude" + assert result.allowed_tools == "read-file write-file" + assert result.metadata == {"author": "test-author", "version": "2.0"} + + async def test_file_skill_fields_populated_from_discovery(self, tmp_path: Path) -> None: + """End-to-end: spec fields are populated on FileSkill via discovery.""" + skill_dir = tmp_path / "test-skill" + skill_dir.mkdir() + skill_md = skill_dir / "SKILL.md" + skill_md.write_text( + "---\n" + "name: test-skill\n" + "description: A test skill.\n" + "license: MIT\n" + "compatibility: GPT-4\n" + "allowed-tools: tool1\n" + "metadata:\n" + " key: value\n" + "---\n" + "Instructions.", + encoding="utf-8", + ) + source = FileSkillsSource(str(tmp_path)) + skills = await source.get_skills() + assert len(skills) == 1 + skill = skills[0] + assert isinstance(skill, FileSkill) + assert skill.frontmatter.license == "MIT" + assert skill.frontmatter.compatibility == "GPT-4" + assert skill.frontmatter.allowed_tools == "tool1" + assert skill.frontmatter.metadata == {"key": "value"} + + def test_metadata_children_do_not_override_top_level_fields(self) -> None: + """Indented keys inside a metadata: block must not overwrite top-level fields.""" + content = ( + "---\n" + "name: test-skill\n" + "description: The real description.\n" + "license: MIT\n" + "metadata:\n" + " description: should not override\n" + " license: should not override\n" + " name: should not override\n" + "---\n" + "Body." + ) + result = FileSkillsSource._extract_frontmatter(content, "test.md") + assert result is not None + assert result.name == "test-skill" + assert result.description == "The real description." + assert result.license == "MIT" + assert result.metadata == { + "description": "should not override", + "license": "should not override", + "name": "should not override", + } # --------------------------------------------------------------------------- @@ -1862,7 +2168,7 @@ class TestCreateInstructionsEdgeCases: def test_custom_template_with_literal_braces(self) -> None: skills = [ - InlineSkill(name="my-skill", description="Skill.", instructions="Body"), + InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="Skill."), instructions="Body"), ] template = "Header {{literal}} {skills} footer." result = SkillsProvider._create_instructions(template, skills) @@ -1872,9 +2178,9 @@ class TestCreateInstructionsEdgeCases: def test_multiple_skills_generates_sorted_xml(self) -> None: skills = [ - InlineSkill(name="charlie", description="C.", instructions="Body"), - InlineSkill(name="alpha", description="A.", instructions="Body"), - InlineSkill(name="bravo", description="B.", instructions="Body"), + InlineSkill(frontmatter=SkillFrontmatter(name="charlie", description="C."), instructions="Body"), + InlineSkill(frontmatter=SkillFrontmatter(name="alpha", description="A."), instructions="Body"), + InlineSkill(frontmatter=SkillFrontmatter(name="bravo", description="B."), instructions="Body"), ] result = SkillsProvider._create_instructions(None, skills) assert result is not None @@ -1886,7 +2192,7 @@ class TestCreateInstructionsEdgeCases: def test_custom_template_missing_runner_instructions_raises(self) -> None: """Custom template without {runner_instructions} raises when scripts are enabled.""" skills = [ - InlineSkill(name="my-skill", description="Skill.", instructions="Body"), + InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="Skill."), instructions="Body"), ] template = "Skills: {skills}" with pytest.raises(ValueError, match="runner_instructions"): @@ -1895,7 +2201,7 @@ class TestCreateInstructionsEdgeCases: def test_custom_template_missing_resource_instructions_raises(self) -> None: """Custom template without {resource_instructions} raises when resources exist.""" skills = [ - InlineSkill(name="my-skill", description="Skill.", instructions="Body"), + InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="Skill."), instructions="Body"), ] template = "Skills: {skills}" with pytest.raises(ValueError, match="resource_instructions"): @@ -1904,7 +2210,7 @@ class TestCreateInstructionsEdgeCases: def test_include_resource_instructions_true_adds_resource_text(self) -> None: """When include_resource_instructions is True, resource instructions appear in the prompt.""" skills = [ - InlineSkill(name="my-skill", description="Skill.", instructions="Body"), + InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="Skill."), instructions="Body"), ] result = SkillsProvider._create_instructions(None, skills, include_resource_instructions=True) assert result is not None @@ -1913,7 +2219,7 @@ class TestCreateInstructionsEdgeCases: def test_include_resource_instructions_false_omits_resource_text(self) -> None: """When include_resource_instructions is False, resource instructions do not appear.""" skills = [ - InlineSkill(name="my-skill", description="Skill.", instructions="Body"), + InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="Skill."), instructions="Body"), ] result = SkillsProvider._create_instructions(None, skills, include_resource_instructions=False) assert result is not None @@ -1922,7 +2228,7 @@ class TestCreateInstructionsEdgeCases: def test_custom_template_with_unknown_placeholder_raises(self) -> None: """Template with an unknown placeholder raises ValueError.""" skills = [ - InlineSkill(name="my-skill", description="Skill.", instructions="Body"), + InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="Skill."), instructions="Body"), ] template = "Skills: {skills} {unknown_key}" with pytest.raises(ValueError, match="valid format string"): @@ -1952,7 +2258,7 @@ class TestSkillsProviderEdgeCases: assert "empty" in result async def test_read_skill_resource_whitespace_skill_name_returns_error(self) -> None: - skill = InlineSkill(name="my-skill", description="A skill.", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), instructions="Body") provider = SkillsProvider([skill]) await _init_provider(provider) result = await provider._read_skill_resource(_raw_skills(provider), " ", "ref") @@ -1960,7 +2266,7 @@ class TestSkillsProviderEdgeCases: assert "empty" in result async def test_read_skill_resource_whitespace_resource_name_returns_error(self) -> None: - skill = InlineSkill(name="my-skill", description="A skill.", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), instructions="Body") provider = SkillsProvider([skill]) await _init_provider(provider) result = await provider._read_skill_resource(_raw_skills(provider), "my-skill", " ") @@ -1968,7 +2274,7 @@ class TestSkillsProviderEdgeCases: assert "empty" in result async def test_read_callable_resource_exception_returns_error(self) -> None: - skill = InlineSkill(name="my-skill", description="A skill.", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), instructions="Body") @skill.resource def exploding_resource() -> Any: @@ -1981,7 +2287,7 @@ class TestSkillsProviderEdgeCases: assert "Failed to read resource" in result async def test_read_async_callable_resource_exception_returns_error(self) -> None: - skill = InlineSkill(name="my-skill", description="A skill.", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), instructions="Body") @skill.resource async def async_exploding() -> Any: @@ -1993,7 +2299,9 @@ class TestSkillsProviderEdgeCases: assert result.startswith("Error:") async def test_load_code_skill_xml_escapes_metadata(self) -> None: - skill = InlineSkill(name="my-skill", description='Uses & "quotes"', instructions="Body") + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="my-skill", description='Uses & "quotes"'), instructions="Body" + ) provider = SkillsProvider([skill]) await _init_provider(provider) result = provider._load_skill(_raw_skills(provider), "my-skill") @@ -2001,16 +2309,18 @@ class TestSkillsProviderEdgeCases: assert "&" in result async def test_code_skill_deduplication(self) -> None: - skill1 = InlineSkill(name="my-skill", description="First.", instructions="Body 1") - skill2 = InlineSkill(name="my-skill", description="Second.", instructions="Body 2") + skill1 = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="First."), instructions="Body 1") + skill2 = InlineSkill( + frontmatter=SkillFrontmatter(name="my-skill", description="Second."), instructions="Body 2" + ) provider = SkillsProvider([skill1, skill2]) await _init_provider(provider) assert len(_ctx(provider)[0]) == 1 - assert "First." in _ctx(provider)[0]["my-skill"].description + assert "First." in _ctx(provider)[0]["my-skill"].frontmatter.description async def test_before_run_extends_tools_even_without_instructions(self) -> None: """If instructions are somehow None but skills exist, tools should still be added.""" - skill = InlineSkill(name="my-skill", description="A skill.", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), instructions="Body") provider = SkillsProvider([skill]) context = SessionContext(input_messages=[]) @@ -2122,7 +2432,7 @@ class TestSkillResourceDecoratorEdgeCases: """Additional edge-case tests for the @skill.resource decorator.""" def test_decorator_no_docstring_description_is_none(self) -> None: - skill = InlineSkill(name="my-skill", description="A skill.", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), instructions="Body") @skill.resource def no_docs() -> Any: @@ -2131,7 +2441,7 @@ class TestSkillResourceDecoratorEdgeCases: assert skill.resources[0].description is None def test_decorator_with_name_only(self) -> None: - skill = InlineSkill(name="my-skill", description="A skill.", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), instructions="Body") @skill.resource(name="custom-name") def get_data() -> Any: @@ -2143,7 +2453,7 @@ class TestSkillResourceDecoratorEdgeCases: assert skill.resources[0].description is None def test_decorator_with_description_only(self) -> None: - skill = InlineSkill(name="my-skill", description="A skill.", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), instructions="Body") @skill.resource(description="Custom desc") def get_data() -> Any: @@ -2153,7 +2463,7 @@ class TestSkillResourceDecoratorEdgeCases: assert skill.resources[0].description == "Custom desc" def test_decorator_preserves_original_function_identity(self) -> None: - skill = InlineSkill(name="my-skill", description="A skill.", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="A skill."), instructions="Body") @skill.resource def original() -> Any: @@ -2230,7 +2540,7 @@ class TestSkillScriptRun: return f"hello {name}" script = InlineSkillScript(name="greet", function=greet) - skill = InlineSkill(name="s", description="d", instructions="c") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="s", description="d"), instructions="c") result = await script.run(skill, args={"name": "Alice"}) assert result == "hello Alice" @@ -2239,7 +2549,7 @@ class TestSkillScriptRun: return f"async {name}" script = InlineSkillScript(name="greet", function=greet) - skill = InlineSkill(name="s", description="d", instructions="c") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="s", description="d"), instructions="c") result = await script.run(skill, args={"name": "Bob"}) assert result == "async Bob" @@ -2248,13 +2558,13 @@ class TestSkillScriptRun: return {"x": x, **kwargs} script = InlineSkillScript(name="f", function=func) - skill = InlineSkill(name="s", description="d", instructions="c") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="s", description="d"), instructions="c") result = await script.run(skill, args={"x": 1}, extra="val") assert result == {"x": 1, "extra": "val"} async def test_run_code_defined_no_args(self) -> None: script = InlineSkillScript(name="f", function=lambda: 42) - skill = InlineSkill(name="s", description="d", instructions="c") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="s", description="d"), instructions="c") result = await script.run(skill) assert result == 42 @@ -2262,13 +2572,15 @@ class TestSkillScriptRun: captured: dict[str, Any] = {} def runner(skill: Skill, script: SkillScript, args: dict[str, Any] | None = None) -> str: - captured["skill"] = skill.name + captured["skill"] = skill.frontmatter.name captured["script"] = script.name captured["args"] = args return "runner_result" script = FileSkillScript(name="run.py", full_path=f"{_ABS}/test/run.py", runner=runner) - skill = FileSkill(name="my-skill", description="d", content="c", path=f"{_ABS}/test") + skill = FileSkill( + frontmatter=SkillFrontmatter(name="my-skill", description="d"), content="c", path=f"{_ABS}/test" + ) result = await script.run(skill, args={"key": "val"}) assert result == "runner_result" assert captured["skill"] == "my-skill" @@ -2280,19 +2592,19 @@ class TestSkillScriptRun: return "async_runner" script = FileSkillScript(name="run.py", full_path=f"{_ABS}/test/run.py", runner=runner) - skill = FileSkill(name="s", description="d", content="c", path=f"{_ABS}/test") + skill = FileSkill(frontmatter=SkillFrontmatter(name="s", description="d"), content="c", path=f"{_ABS}/test") result = await script.run(skill, args=None) assert result == "async_runner" async def test_run_file_based_without_runner_raises(self) -> None: script = FileSkillScript(name="run.py", full_path=f"{_ABS}/test/run.py") - skill = FileSkill(name="s", description="d", content="c", path=f"{_ABS}/test") + skill = FileSkill(frontmatter=SkillFrontmatter(name="s", description="d"), content="c", path=f"{_ABS}/test") with pytest.raises(ValueError, match="requires a runner"): await script.run(skill) async def test_run_file_based_with_non_file_skill_raises_type_error(self) -> None: script = FileSkillScript(name="run.py", full_path=f"{_ABS}/test/run.py", runner=_noop_script_runner) - skill = InlineSkill(name="s", description="d", instructions="c") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="s", description="d"), instructions="c") with pytest.raises(TypeError, match="requires a FileSkill"): await script.run(skill) @@ -2314,7 +2626,7 @@ class TestSkillScriptDecorator: """Tests for the @skill.script decorator.""" def test_bare_decorator(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") @skill.script def analyze(query: str) -> str: @@ -2328,7 +2640,7 @@ class TestSkillScriptDecorator: assert skill.scripts[0].function is analyze def test_parameterized_decorator(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") @skill.script(name="custom-name", description="Custom desc") def my_func() -> str: @@ -2341,7 +2653,7 @@ class TestSkillScriptDecorator: assert skill.scripts[0].function is my_func def test_multiple_scripts(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") @skill.script def script_a() -> str: @@ -2356,7 +2668,7 @@ class TestSkillScriptDecorator: assert skill.scripts[1].name == "script_b" def test_async_script(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") @skill.script async def fetch_data() -> str: @@ -2369,7 +2681,7 @@ class TestSkillScriptDecorator: assert skill.scripts[0].function is fetch_data def test_decorator_returns_original_function(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") @skill.script def original() -> str: @@ -2392,12 +2704,14 @@ class TestSkillWithScripts: """Tests for the Skill class with scripts attribute.""" def test_default_empty_scripts(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") assert skill.scripts == [] def test_scripts_at_construction(self) -> None: scripts = [InlineSkillScript(name="s1", function=lambda: None)] - skill = InlineSkill(name="my-skill", description="test", instructions="body", scripts=scripts) + skill = InlineSkill( + frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body", scripts=scripts + ) assert len(skill.scripts) == 1 assert skill.scripts[0].name == "s1" @@ -2414,12 +2728,12 @@ class TestSkillScriptRunnerProtocol: results: list[tuple] = [] async def my_runner(skill, script, args=None): - results.append((skill.name, script.name, args)) + results.append((skill.frontmatter.name, script.name, args)) return "executed" assert isinstance(my_runner, SkillScriptRunner) - skill = InlineSkill(name="test-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="test"), instructions="body") script = FileSkillScript(name="my-script", full_path=f"{_ABS}/test/scripts/run.py") skill.scripts.append(script) @@ -2437,7 +2751,7 @@ class TestSkillScriptRunnerProtocol: runner = _CustomRunner() assert isinstance(runner, SkillScriptRunner) - skill = InlineSkill(name="test-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="test"), instructions="body") script = InlineSkillScript(name="my-script", function=lambda: None) skill.scripts.append(script) @@ -2448,7 +2762,7 @@ class TestSkillScriptRunnerProtocol: async def noop_runner(skill, script, args=None): return None - skill = InlineSkill(name="test-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="test"), instructions="body") script = InlineSkillScript(name="s1", function=lambda: None) result = await noop_runner(skill, script) @@ -2458,7 +2772,7 @@ class TestSkillScriptRunnerProtocol: async def dict_runner(skill, script, args=None): return {"exit_code": 0, "output": "ok"} - skill = InlineSkill(name="test-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="test"), instructions="body") script = FileSkillScript(name="s1", full_path=f"{_ABS}/test/scripts/run.py") result = await dict_runner(skill, script) @@ -2468,12 +2782,12 @@ class TestSkillScriptRunnerProtocol: results: list[tuple] = [] def my_runner(skill, script, args=None): - results.append((skill.name, script.name, args)) + results.append((skill.frontmatter.name, script.name, args)) return "executed" assert isinstance(my_runner, SkillScriptRunner) - skill = InlineSkill(name="test-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="test"), instructions="body") script = FileSkillScript(name="my-script", full_path=f"{_ABS}/test/scripts/run.py") skill.scripts.append(script) @@ -2491,7 +2805,7 @@ class TestSkillScriptRunnerProtocol: runner = _SyncRunner() assert isinstance(runner, SkillScriptRunner) - skill = InlineSkill(name="test-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="test"), instructions="body") script = InlineSkillScript(name="my-script", function=lambda: None) skill.scripts.append(script) @@ -2502,7 +2816,7 @@ class TestSkillScriptRunnerProtocol: def noop_runner(skill, script, args=None): return None - skill = InlineSkill(name="test-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="test"), instructions="body") script = InlineSkillScript(name="s1", function=lambda: None) result = noop_runner(skill, script) @@ -2512,7 +2826,7 @@ class TestSkillScriptRunnerProtocol: def dict_runner(skill, script, args=None): return {"exit_code": 0, "output": "ok"} - skill = InlineSkill(name="test-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="test"), instructions="body") script = FileSkillScript(name="s1", full_path=f"{_ABS}/test/scripts/run.py") result = dict_runner(skill, script) @@ -2528,7 +2842,7 @@ class TestSkillsProviderFactories: """Tests for the SkillsProvider constructor auto-wiring behavior.""" async def test_code_skills_with_scripts_creates_provider(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=lambda: None)) provider = SkillsProvider([skill]) @@ -2538,7 +2852,7 @@ class TestSkillsProviderFactories: assert any(hasattr(t, "name") and t.name == "run_skill_script" for t in _ctx(provider)[2]) async def test_code_skills_no_scripts(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") provider = SkillsProvider([skill]) await _init_provider(provider) # No scripts with functions, no runner, no resources — only load_skill @@ -2549,7 +2863,7 @@ class TestSkillsProviderFactories: def my_function(key: str = "") -> str: return f"executed: {key}" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=my_function)) provider = SkillsProvider([skill]) @@ -2560,7 +2874,7 @@ class TestSkillsProviderFactories: assert result == "executed: hello" async def test_no_scripts_no_tool(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") # No scripts at all — no run_skill_script tool provider = SkillsProvider([skill]) await _init_provider(provider) @@ -2568,14 +2882,14 @@ class TestSkillsProviderFactories: async def test_no_resources_no_read_skill_resource_tool(self) -> None: """When no skill has resources, read_skill_resource tool is not advertised.""" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") provider = SkillsProvider([skill]) await _init_provider(provider) assert not any(hasattr(t, "name") and t.name == "read_skill_resource" for t in _ctx(provider)[2]) async def test_resources_present_includes_read_skill_resource_tool(self) -> None: """When a skill has resources, read_skill_resource tool is advertised.""" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.resources.append(InlineSkillResource(name="ref", content="reference data")) provider = SkillsProvider([skill]) await _init_provider(provider) @@ -2583,7 +2897,7 @@ class TestSkillsProviderFactories: async def test_resources_present_includes_resource_instructions(self) -> None: """When a skill has resources, instructions mention read_skill_resource.""" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.resources.append(InlineSkillResource(name="ref", content="reference data")) provider = SkillsProvider([skill]) await _init_provider(provider) @@ -2591,14 +2905,14 @@ class TestSkillsProviderFactories: async def test_no_resources_excludes_resource_instructions(self) -> None: """When no skill has resources, instructions do not mention read_skill_resource.""" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") provider = SkillsProvider([skill]) await _init_provider(provider) assert "read_skill_resource" not in (_ctx(provider)[1] or "") async def test_read_skill_resource_tool_returns_content(self) -> None: """The read_skill_resource tool returns resource content when invoked.""" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.resources.append(InlineSkillResource(name="ref", content="reference data")) provider = SkillsProvider([skill]) await _init_provider(provider) @@ -2695,7 +3009,9 @@ class TestSkillsProviderFactories: encoding="utf-8", ) - code_skill = InlineSkill(name="code-skill", description="test", instructions="body") + code_skill = InlineSkill( + frontmatter=SkillFrontmatter(name="code-skill", description="test"), instructions="body" + ) code_skill.scripts.append(InlineSkillScript(name="s1", function=lambda: None)) provider = SkillsProvider( @@ -2725,7 +3041,7 @@ class TestSkillsProviderFactories: async def test_file_script_error_without_runner(self) -> None: # A skill with both a code script and a file-based script - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="code-s", function=lambda: "ok")) skill.scripts.append(FileSkillScript(name="file-s", full_path=f"{_ABS}/test/scripts/s1.py")) @@ -2746,7 +3062,7 @@ class TestSkillsProviderFactories: async def async_func(x: int = 0) -> str: return f"async: {x}" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=async_func)) provider = SkillsProvider([skill]) @@ -2761,7 +3077,7 @@ class TestSkillsProviderFactories: def returns_dict() -> dict: return {"status": "ok", "value": 42} - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=returns_dict)) provider = SkillsProvider([skill]) @@ -2772,7 +3088,7 @@ class TestSkillsProviderFactories: async def test_code_script_returns_none(self) -> None: """Code-defined scripts returning None pass through as None.""" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=lambda: None)) provider = SkillsProvider([skill]) @@ -2783,7 +3099,7 @@ class TestSkillsProviderFactories: async def test_script_with_path_errors_without_runner(self) -> None: """A file-based script without a runner should return an error.""" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="code-s", function=lambda: "ok")) skill.scripts.append(FileSkillScript(name="path-s", full_path=f"{_ABS}/test/scripts/s1.py")) @@ -2801,7 +3117,7 @@ class TestSkillsProviderFactories: assert "script_runner" in result or "Failed to run" in result async def test_run_skill_script_error_on_missing_skill(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=lambda: None)) provider = SkillsProvider([skill]) @@ -2812,7 +3128,7 @@ class TestSkillsProviderFactories: assert "nonexistent" in result async def test_run_skill_script_sync_with_kwargs(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") @skill.script def greet(name: str, **kwargs: Any) -> str: @@ -2827,7 +3143,7 @@ class TestSkillsProviderFactories: assert result == "Hello Alice (user=u42)" async def test_run_skill_script_async_with_kwargs(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") @skill.script async def fetch(url: str, **kwargs: Any) -> str: @@ -2843,7 +3159,7 @@ class TestSkillsProviderFactories: async def test_run_skill_script_without_kwargs_ignores_extra_args(self) -> None: """Script functions without **kwargs should still work when runtime kwargs are passed.""" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") @skill.script def simple(query: str) -> str: @@ -2858,7 +3174,7 @@ class TestSkillsProviderFactories: async def test_run_skill_script_conflicting_args_and_kwargs_raises(self) -> None: """Conflicting keys in args and kwargs should raise TypeError.""" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") @skill.script def process(**kwargs: Any) -> str: @@ -2872,7 +3188,7 @@ class TestSkillsProviderFactories: assert "Error" in result async def test_run_skill_script_error_on_missing_script(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=lambda: None)) provider = SkillsProvider([skill]) @@ -2883,7 +3199,7 @@ class TestSkillsProviderFactories: assert "nonexistent" in result async def test_run_skill_script_error_on_empty_names(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=lambda: None)) provider = SkillsProvider([skill]) @@ -2897,7 +3213,7 @@ class TestSkillsProviderFactories: assert "Error" in result async def test_instructions_include_script_runner_hints(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=lambda: None)) provider = SkillsProvider([skill]) @@ -2906,14 +3222,14 @@ class TestSkillsProviderFactories: assert "not as top-level tool parameters" in _ctx(provider)[1] async def test_no_scripts_no_runner_no_script_instructions(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") provider = SkillsProvider([skill]) await _init_provider(provider) # No scripts and no runner — instructions should not mention run_skill_script assert "run_skill_script" not in (_ctx(provider)[1] or "") async def test_tool_schema_args_description_mentions_key_format(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=lambda: None)) provider = SkillsProvider([skill]) @@ -2925,7 +3241,7 @@ class TestSkillsProviderFactories: async def test_require_script_approval_sets_approval_mode(self) -> None: """When require_script_approval=True, the run_skill_script tool has approval_mode='always_require'.""" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=lambda: None)) provider = SkillsProvider([skill], require_script_approval=True) @@ -2935,7 +3251,7 @@ class TestSkillsProviderFactories: async def test_require_script_approval_false_by_default(self) -> None: """By default, the run_skill_script tool has approval_mode='never_require'.""" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=lambda: None)) provider = SkillsProvider([skill]) @@ -2945,7 +3261,7 @@ class TestSkillsProviderFactories: async def test_require_script_approval_does_not_affect_other_tools(self) -> None: """The load_skill tool should never require approval.""" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=lambda: None)) provider = SkillsProvider([skill], require_script_approval=True) @@ -2961,7 +3277,7 @@ class TestSkillsProviderFactories: def failing_script() -> str: raise RuntimeError("Something went wrong") - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="boom", function=failing_script)) provider = SkillsProvider([skill]) @@ -2974,7 +3290,7 @@ class TestSkillsProviderFactories: async def test_custom_template_without_runner_placeholder_raises(self) -> None: """Provider with code scripts and custom template missing {runner_instructions} raises.""" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=lambda: None)) provider = SkillsProvider( @@ -3138,7 +3454,7 @@ class TestCreateInstructionsWithScripts: """Tests for script metadata in skill advertisement.""" def test_excludes_script_count(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=lambda: None)) result = SkillsProvider._create_instructions(None, [skill]) @@ -3146,7 +3462,7 @@ class TestCreateInstructionsWithScripts: assert "" not in result def test_no_scripts_element_when_empty(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") result = SkillsProvider._create_instructions(None, [skill]) assert result is not None @@ -3162,7 +3478,7 @@ class TestLoadSkillWithScripts: """Tests for script metadata in load_skill output.""" async def test_code_skill_includes_scripts_element(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="analyze", description="Run analysis", function=lambda: None)) provider = SkillsProvider([skill]) @@ -3174,7 +3490,7 @@ class TestLoadSkillWithScripts: assert 'description="Run analysis"' in result async def test_code_skill_no_scripts_element(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") provider = SkillsProvider([skill]) await _init_provider(provider) result = provider._load_skill(_raw_skills(provider), "my-skill") @@ -3190,7 +3506,7 @@ class _MinimalClassSkill(ClassSkill): """A minimal class-based skill with no resources or scripts.""" def __init__(self) -> None: - super().__init__(name="minimal-skill", description="A minimal skill.") + super().__init__(frontmatter=SkillFrontmatter(name="minimal-skill", description="A minimal skill.")) @property def instructions(self) -> str: @@ -3201,7 +3517,7 @@ class _FullClassSkill(ClassSkill): """A class-based skill with resources and scripts.""" def __init__(self) -> None: - super().__init__(name="full-skill", description="A full skill.") + super().__init__(frontmatter=SkillFrontmatter(name="full-skill", description="A full skill.")) self._resources: list[SkillResource] | None = None self._scripts: list[SkillScript] | None = None @@ -3318,7 +3634,7 @@ class TestClassSkill: skills = _raw_skills(provider) assert len(skills) == 1 - assert skills[0].name == "full-skill" + assert skills[0].frontmatter.name == "full-skill" async def test_provider_loads_class_skill_content(self) -> None: skill = _FullClassSkill() @@ -3335,16 +3651,18 @@ class TestClassSkill: source = InMemorySkillsSource([skill]) skills = await source.get_skills() assert len(skills) == 1 - assert skills[0].name == "minimal-skill" + assert skills[0].frontmatter.name == "minimal-skill" async def test_mixed_inline_and_class_skills(self) -> None: - inline = InlineSkill(name="inline-skill", description="Inline", instructions="inline body") + inline = InlineSkill( + frontmatter=SkillFrontmatter(name="inline-skill", description="Inline"), instructions="inline body" + ) class_skill = _MinimalClassSkill() provider = SkillsProvider([inline, class_skill]) await _init_provider(provider) skills = _raw_skills(provider) - names = {s.name for s in skills} + names = {s.frontmatter.name for s in skills} assert names == {"inline-skill", "minimal-skill"} async def test_class_skill_script_runs(self) -> None: @@ -3372,7 +3690,9 @@ class _DecoratorClassSkill(ClassSkill): """A class-based skill using @ClassSkill.resource and @ClassSkill.script decorators.""" def __init__(self) -> None: - super().__init__(name="decorator-skill", description="A decorator-discovered skill.") + super().__init__( + frontmatter=SkillFrontmatter(name="decorator-skill", description="A decorator-discovered skill.") + ) @property def instructions(self) -> str: @@ -3395,7 +3715,7 @@ class _BareDecoratorSkill(ClassSkill): """Skill using bare decorators (no arguments) — name/description from method.""" def __init__(self) -> None: - super().__init__(name="bare-skill", description="Bare decorator skill.") + super().__init__(frontmatter=SkillFrontmatter(name="bare-skill", description="Bare decorator skill.")) @property def instructions(self) -> str: @@ -3416,7 +3736,7 @@ class _DuplicateResourceSkill(ClassSkill): """Skill with duplicate resource names — should raise.""" def __init__(self) -> None: - super().__init__(name="dup-skill", description="Dup.") + super().__init__(frontmatter=SkillFrontmatter(name="dup-skill", description="Dup.")) @property def instructions(self) -> str: @@ -3435,7 +3755,7 @@ class _DuplicateScriptSkill(ClassSkill): """Skill with duplicate script names — should raise.""" def __init__(self) -> None: - super().__init__(name="dup-script-skill", description="Dup.") + super().__init__(frontmatter=SkillFrontmatter(name="dup-script-skill", description="Dup.")) @property def instructions(self) -> str: @@ -3454,7 +3774,7 @@ class _SelfAccessSkill(ClassSkill): """Skill where resource/script access instance state via self.""" def __init__(self, multiplier: int = 10) -> None: - super().__init__(name="self-access", description="Self access skill.") + super().__init__(frontmatter=SkillFrontmatter(name="self-access", description="Self access skill.")) self.multiplier = multiplier @property @@ -3581,7 +3901,7 @@ class TestClassSkillDecoratorDiscovery: skills = _raw_skills(provider) assert len(skills) == 1 - assert skills[0].name == "decorator-skill" + assert skills[0].frontmatter.name == "decorator-skill" def test_manual_override_wins(self) -> None: """A subclass that overrides resources/scripts bypasses decorator discovery.""" @@ -3688,7 +4008,7 @@ class TestClassSkillDecoratorDiscovery: class _BadOrder(ClassSkill): def __init__(self) -> None: - super().__init__(name="bad", description="bad") + super().__init__(frontmatter=SkillFrontmatter(name="bad", description="bad")) @property def instructions(self) -> str: @@ -3705,7 +4025,7 @@ class TestClassSkillDecoratorDiscovery: class _BadOrder(ClassSkill): def __init__(self) -> None: - super().__init__(name="bad", description="bad") + super().__init__(frontmatter=SkillFrontmatter(name="bad", description="bad")) @property def instructions(self) -> str: @@ -3722,7 +4042,7 @@ class TestClassSkillDecoratorDiscovery: class _BadName(ClassSkill): def __init__(self) -> None: - super().__init__(name="bad", description="bad") + super().__init__(frontmatter=SkillFrontmatter(name="bad", description="bad")) @property def instructions(self) -> str: @@ -3738,7 +4058,7 @@ class TestClassSkillDecoratorDiscovery: class _BadName(ClassSkill): def __init__(self) -> None: - super().__init__(name="bad", description="bad") + super().__init__(frontmatter=SkillFrontmatter(name="bad", description="bad")) @property def instructions(self) -> str: @@ -3754,7 +4074,7 @@ class TestClassSkillDecoratorDiscovery: class _EmptyName(ClassSkill): def __init__(self) -> None: - super().__init__(name="bad", description="bad") + super().__init__(frontmatter=SkillFrontmatter(name="bad", description="bad")) @property def instructions(self) -> str: @@ -3798,7 +4118,7 @@ class _ExplicitDescriptionSkill(ClassSkill): """Skill with explicit descriptions on decorator.""" def __init__(self) -> None: - super().__init__(name="desc-skill", description="Explicit desc.") + super().__init__(frontmatter=SkillFrontmatter(name="desc-skill", description="Explicit desc.")) @property def instructions(self) -> str: @@ -3817,7 +4137,7 @@ class _PropertyCallCountSkill(ClassSkill): """Tracks how many times the property getter is called.""" def __init__(self) -> None: - super().__init__(name="callcount-skill", description="Tracks calls.") + super().__init__(frontmatter=SkillFrontmatter(name="callcount-skill", description="Tracks calls.")) self.getter_call_count = 0 @property @@ -3847,7 +4167,7 @@ class _ChildSkill(_ParentSkill): """Child inheriting parent resources and adding its own.""" def __init__(self) -> None: - super().__init__(name="child-skill", description="Child.") + super().__init__(frontmatter=SkillFrontmatter(name="child-skill", description="Child.")) @property def instructions(self) -> str: @@ -3862,7 +4182,7 @@ class _KwargsSkill(ClassSkill): """Skill that uses **kwargs from runtime.""" def __init__(self) -> None: - super().__init__(name="kwargs-skill", description="Kwargs.") + super().__init__(frontmatter=SkillFrontmatter(name="kwargs-skill", description="Kwargs.")) @property def instructions(self) -> str: @@ -3886,7 +4206,7 @@ class _ChildWithInheritedPropertySkill(_ParentWithPropertyResource): """Child that should discover inherited property resource.""" def __init__(self) -> None: - super().__init__(name="child-prop-skill", description="Child prop.") + super().__init__(frontmatter=SkillFrontmatter(name="child-prop-skill", description="Child prop.")) @property def instructions(self) -> str: @@ -3897,7 +4217,7 @@ class _PropertyResourceSkill(ClassSkill): """Skill with a property-based resource.""" def __init__(self) -> None: - super().__init__(name="prop-skill", description="Property skill.") + super().__init__(frontmatter=SkillFrontmatter(name="prop-skill", description="Property skill.")) @property def instructions(self) -> str: @@ -3914,7 +4234,7 @@ class _MixedPropertyMethodSkill(ClassSkill): """Skill with both property and method resources.""" def __init__(self) -> None: - super().__init__(name="mixed-prop", description="Mixed.") + super().__init__(frontmatter=SkillFrontmatter(name="mixed-prop", description="Mixed.")) @property def instructions(self) -> str: @@ -3937,7 +4257,7 @@ class _MixedPropertyMethodSkill(ClassSkill): def analyze(query: str, limit: int = 10) -> str: return "result" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="analyze", description="Run analysis", function=analyze)) provider = SkillsProvider([skill]) @@ -3954,7 +4274,7 @@ class TestReadSkillResourceWithScripts: """Tests for _read_skill_resource falling back to scripts.""" async def test_reads_script_with_static_content(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="generate.py", function=lambda: "print('hello')")) provider = SkillsProvider([skill]) @@ -3964,7 +4284,7 @@ class TestReadSkillResourceWithScripts: assert "not found" in result async def test_script_not_accessible_via_read_resource(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="run.py", function=lambda: "script output")) provider = SkillsProvider([skill]) @@ -3977,7 +4297,7 @@ class TestReadSkillResourceWithScripts: async def async_script() -> str: return "async output" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="run.py", function=async_script)) provider = SkillsProvider([skill]) @@ -3986,7 +4306,7 @@ class TestReadSkillResourceWithScripts: assert "not found" in result async def test_script_case_insensitive_not_in_resources(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="Generate.py", function=lambda: "code")) provider = SkillsProvider([skill]) @@ -3995,7 +4315,7 @@ class TestReadSkillResourceWithScripts: assert "not found" in result async def test_resource_takes_priority_over_script(self) -> None: - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.resources.append(InlineSkillResource(name="data.py", content="resource content")) skill.scripts.append(InlineSkillScript(name="data.py", function=lambda: "script content")) @@ -4008,7 +4328,7 @@ class TestReadSkillResourceWithScripts: def failing_script() -> str: raise RuntimeError("boom") - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="bad.py", function=failing_script)) provider = SkillsProvider([skill]) @@ -4217,7 +4537,7 @@ class TestLoadSkillsMerging: def test_code_skill_with_invalid_name_raises(self) -> None: """Code skills with invalid metadata (e.g. uppercase name) raise at construction.""" with pytest.raises(ValueError, match="Invalid skill name"): - InlineSkill(name="INVALID_NAME", description="valid", instructions="body") + InlineSkill(frontmatter=SkillFrontmatter(name="INVALID_NAME", description="valid"), instructions="body") async def test_file_skill_takes_precedence_over_code_skill(self, tmp_path: Path) -> None: """When file-based and code-defined skills share a name, file-based wins.""" @@ -4235,7 +4555,9 @@ class TestLoadSkillsMerging: encoding="utf-8", ) - code_skill = InlineSkill(name="my-skill", description="Code skill.", instructions="Code body.") + code_skill = InlineSkill( + frontmatter=SkillFrontmatter(name="my-skill", description="Code skill."), instructions="Code body." + ) source = DeduplicatingSkillsSource( AggregatingSkillsSource([ @@ -4244,7 +4566,7 @@ class TestLoadSkillsMerging: ]) ) result = await source.get_skills() - skills_by_name = {s.name: s for s in result} + skills_by_name = {s.frontmatter.name: s for s in result} assert "my-skill" in skills_by_name assert skills_by_name["my-skill"].path is not None # file-based skill has path set @@ -4269,7 +4591,7 @@ class TestSkillsSource: source = FileSkillsSource(str(tmp_path)) skills = await source.get_skills() assert len(skills) == 1 - assert skills[0].name == "my-skill" + assert skills[0].frontmatter.name == "my-skill" assert skills[0].path is not None async def test_file_skills_source_with_extensions(self, tmp_path: Path) -> None: @@ -4295,67 +4617,67 @@ class TestSkillsSource: """InMemorySkillsSource returns all provided skills.""" from agent_framework import InMemorySkillsSource - s1 = InlineSkill(name="skill-a", description="A", instructions="body") - s2 = InlineSkill(name="skill-b", description="B", instructions="body") + s1 = InlineSkill(frontmatter=SkillFrontmatter(name="skill-a", description="A"), instructions="body") + s2 = InlineSkill(frontmatter=SkillFrontmatter(name="skill-b", description="B"), instructions="body") source = InMemorySkillsSource([s1, s2]) skills = await source.get_skills() assert len(skills) == 2 - assert skills[0].name == "skill-a" - assert skills[1].name == "skill-b" + assert skills[0].frontmatter.name == "skill-a" + assert skills[1].frontmatter.name == "skill-b" async def test_aggregating_source_combines_sources(self) -> None: """Aggregating source concatenates results from multiple sources.""" from agent_framework import AggregatingSkillsSource, InMemorySkillsSource - s1 = InlineSkill(name="skill-a", description="A", instructions="body") - s2 = InlineSkill(name="skill-b", description="B", instructions="body") + s1 = InlineSkill(frontmatter=SkillFrontmatter(name="skill-a", description="A"), instructions="body") + s2 = InlineSkill(frontmatter=SkillFrontmatter(name="skill-b", description="B"), instructions="body") source = AggregatingSkillsSource([ InMemorySkillsSource([s1]), InMemorySkillsSource([s2]), ]) skills = await source.get_skills() - names = [s.name for s in skills] + names = [s.frontmatter.name for s in skills] assert names == ["skill-a", "skill-b"] async def test_filtering_source_filters_by_predicate(self) -> None: """FilteringSkillsSource only returns skills matching the predicate.""" from agent_framework import FilteringSkillsSource, InMemorySkillsSource - s1 = InlineSkill(name="keep-me", description="keep", instructions="body") - s2 = InlineSkill(name="drop-me", description="drop", instructions="body") + s1 = InlineSkill(frontmatter=SkillFrontmatter(name="keep-me", description="keep"), instructions="body") + s2 = InlineSkill(frontmatter=SkillFrontmatter(name="drop-me", description="drop"), instructions="body") source = FilteringSkillsSource( InMemorySkillsSource([s1, s2]), - predicate=lambda s: s.name.startswith("keep"), + predicate=lambda s: s.frontmatter.name.startswith("keep"), ) skills = await source.get_skills() assert len(skills) == 1 - assert skills[0].name == "keep-me" + assert skills[0].frontmatter.name == "keep-me" async def test_deduplicating_source_removes_duplicates(self) -> None: """DeduplicatingSkillsSource keeps first skill with each name.""" from agent_framework import DeduplicatingSkillsSource, InMemorySkillsSource - s1 = InlineSkill(name="my-skill", description="first", instructions="body1") - s2 = InlineSkill(name="my-skill", description="second", instructions="body2") - s3 = InlineSkill(name="other", description="other", instructions="body3") + s1 = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="first"), instructions="body1") + s2 = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="second"), instructions="body2") + s3 = InlineSkill(frontmatter=SkillFrontmatter(name="other", description="other"), instructions="body3") source = DeduplicatingSkillsSource(InMemorySkillsSource([s1, s2, s3])) skills = await source.get_skills() assert len(skills) == 2 - names = {s.name for s in skills} + names = {s.frontmatter.name for s in skills} assert names == {"my-skill", "other"} # First one wins - my_skill = next(s for s in skills if s.name == "my-skill") - assert my_skill.description == "first" + my_skill = next(s for s in skills if s.frontmatter.name == "my-skill") + assert my_skill.frontmatter.description == "first" async def test_delegating_source_delegates(self) -> None: """DelegatingSkillsSource delegates to inner source by default.""" from agent_framework import DelegatingSkillsSource, InMemorySkillsSource - skill = InlineSkill(name="test-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="test"), instructions="body") inner = InMemorySkillsSource([skill]) class PassthroughSource(DelegatingSkillsSource): @@ -4365,7 +4687,7 @@ class TestSkillsSource: assert source.inner_source is inner skills = await source.get_skills() assert len(skills) == 1 - assert skills[0].name == "test-skill" + assert skills[0].frontmatter.name == "test-skill" async def test_provider_with_source_parameter(self, tmp_path: Path) -> None: """SkillsProvider works with the new source= parameter.""" @@ -4385,7 +4707,9 @@ class TestSkillsSource: """When source= is provided, skill_paths and skills are ignored.""" from agent_framework import InMemorySkillsSource - code_skill = InlineSkill(name="code-skill", description="test", instructions="body") + code_skill = InlineSkill( + frontmatter=SkillFrontmatter(name="code-skill", description="test"), instructions="body" + ) source = InMemorySkillsSource([code_skill]) # Pass skill_paths that would normally discover file skills — should be ignored @@ -4411,8 +4735,12 @@ class TestSkillsSource: encoding="utf-8", ) - code_skill = InlineSkill(name="code-skill", description="Code.", instructions="Body.") - internal = InlineSkill(name="internal", description="Internal.", instructions="Body.") + code_skill = InlineSkill( + frontmatter=SkillFrontmatter(name="code-skill", description="Code."), instructions="Body." + ) + internal = InlineSkill( + frontmatter=SkillFrontmatter(name="internal", description="Internal."), instructions="Body." + ) source = FilteringSkillsSource( DeduplicatingSkillsSource( @@ -4421,11 +4749,11 @@ class TestSkillsSource: InMemorySkillsSource([code_skill, internal]), ]) ), - predicate=lambda s: s.name != "internal", + predicate=lambda s: s.frontmatter.name != "internal", ) skills = await source.get_skills() - names = {s.name for s in skills} + names = {s.frontmatter.name for s in skills} assert names == {"file-skill", "code-skill"} assert "internal" not in names @@ -4453,15 +4781,15 @@ class TestSourceComposition: async def test_code_skills_with_provider(self) -> None: """InMemorySkillsSource with code skills creates a working provider.""" - skill = InlineSkill(name="code-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="code-skill", description="test"), instructions="body") provider = SkillsProvider(DeduplicatingSkillsSource(InMemorySkillsSource([skill]))) await _init_provider(provider) assert "code-skill" in _ctx(provider)[0] async def test_multiple_code_skills(self) -> None: """InMemorySkillsSource with multiple skills registers them all.""" - s1 = InlineSkill(name="skill-a", description="A", instructions="body") - s2 = InlineSkill(name="skill-b", description="B", instructions="body") + s1 = InlineSkill(frontmatter=SkillFrontmatter(name="skill-a", description="A"), instructions="body") + s2 = InlineSkill(frontmatter=SkillFrontmatter(name="skill-b", description="B"), instructions="body") provider = SkillsProvider(DeduplicatingSkillsSource(InMemorySkillsSource([s1, s2]))) await _init_provider(provider) assert "skill-a" in _ctx(provider)[0] @@ -4469,7 +4797,7 @@ class TestSourceComposition: async def test_custom_source_with_provider(self) -> None: """Custom source passed to SkillsProvider works.""" - skill = InlineSkill(name="custom", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="custom", description="test"), instructions="body") source = InMemorySkillsSource([skill]) provider = SkillsProvider(DeduplicatingSkillsSource(source)) await _init_provider(provider) @@ -4479,13 +4807,13 @@ class TestSourceComposition: """FilteringSkillsSource excludes matching skills.""" from agent_framework import FilteringSkillsSource - s1 = InlineSkill(name="keep-me", description="keep", instructions="body") - s2 = InlineSkill(name="drop-me", description="drop", instructions="body") + s1 = InlineSkill(frontmatter=SkillFrontmatter(name="keep-me", description="keep"), instructions="body") + s2 = InlineSkill(frontmatter=SkillFrontmatter(name="drop-me", description="drop"), instructions="body") source = DeduplicatingSkillsSource( FilteringSkillsSource( InMemorySkillsSource([s1, s2]), - predicate=lambda s: s.name.startswith("keep"), + predicate=lambda s: s.frontmatter.name.startswith("keep"), ) ) provider = SkillsProvider(source) @@ -4495,8 +4823,8 @@ class TestSourceComposition: async def test_dedup_across_sources(self) -> None: """DeduplicatingSkillsSource deduplicates across aggregated sources.""" - s1 = InlineSkill(name="dup", description="first", instructions="body1") - s2 = InlineSkill(name="dup", description="second", instructions="body2") + s1 = InlineSkill(frontmatter=SkillFrontmatter(name="dup", description="first"), instructions="body1") + s2 = InlineSkill(frontmatter=SkillFrontmatter(name="dup", description="second"), instructions="body2") source = DeduplicatingSkillsSource( AggregatingSkillsSource([ @@ -4507,7 +4835,7 @@ class TestSourceComposition: provider = SkillsProvider(source) await _init_provider(provider) assert len(_ctx(provider)[0]) == 1 - assert _ctx(provider)[0]["dup"].description == "first" + assert _ctx(provider)[0]["dup"].frontmatter.description == "first" async def test_file_source_with_script_runner(self, tmp_path: Path) -> None: """FileSkillsSource with script_runner enables script execution.""" @@ -4527,7 +4855,7 @@ class TestSourceComposition: async def test_script_approval_on_provider(self) -> None: """SkillsProvider with require_script_approval sets the approval mode.""" - skill = InlineSkill(name="my-skill", description="test", instructions="body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="test"), instructions="body") skill.scripts.append(InlineSkillScript(name="s1", function=lambda: None)) provider = SkillsProvider( @@ -4616,13 +4944,13 @@ class TestSkillsProviderFactoryMethods: def test_init_with_skills_creates_provider(self) -> None: """Constructor with skill list returns a SkillsProvider instance.""" - skill = InlineSkill(name="test-skill", description="Test", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="Test"), instructions="Body") provider = SkillsProvider([skill]) assert isinstance(provider, SkillsProvider) async def test_init_with_skills_registers_skills(self) -> None: """Constructor with skill list registers code-defined skills.""" - skill = InlineSkill(name="test-skill", description="Test", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="Test"), instructions="Body") provider = SkillsProvider([skill]) await _init_provider(provider) assert "test-skill" in _ctx(provider)[0] @@ -4635,7 +4963,7 @@ class TestSkillsProviderFactoryMethods: async def test_init_with_skills_and_options(self) -> None: """Constructor with skills passes through keyword options.""" - skill = InlineSkill(name="my-skill", description="Test", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="my-skill", description="Test"), instructions="Body") provider = SkillsProvider( [skill], require_script_approval=True, @@ -4648,7 +4976,7 @@ class TestSkillsProviderFactoryMethods: """Constructor with SkillsSource returns a SkillsProvider instance.""" from agent_framework import InMemorySkillsSource - skill = InlineSkill(name="test-skill", description="Test", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="Test"), instructions="Body") source = InMemorySkillsSource([skill]) provider = SkillsProvider(source) assert isinstance(provider, SkillsProvider) @@ -4657,7 +4985,7 @@ class TestSkillsProviderFactoryMethods: """Constructor with SkillsSource uses the exact source given.""" from agent_framework import InMemorySkillsSource - skill = InlineSkill(name="test-skill", description="Test", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="Test"), instructions="Body") source = InMemorySkillsSource([skill]) provider = SkillsProvider(source) await _init_provider(provider) @@ -4674,7 +5002,7 @@ class TestDisableCaching: async def test_default_caching_enabled(self) -> None: """By default, _get_or_create_context only builds once.""" - skill = InlineSkill(name="test-skill", description="Test", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="Test"), instructions="Body") provider = SkillsProvider([skill]) await _init_provider(provider) first_ctx = provider._cached_context # pyright: ignore[reportPrivateUsage] @@ -4686,7 +5014,7 @@ class TestDisableCaching: async def test_disable_caching_rebuilds_on_every_call(self) -> None: """With disable_caching=True, _create_context rebuilds every time.""" - skill = InlineSkill(name="test-skill", description="Test", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="Test"), instructions="Body") provider = SkillsProvider([skill], disable_caching=True) await _init_provider(provider) first_ctx = provider._cached_context # pyright: ignore[reportPrivateUsage] @@ -4700,20 +5028,20 @@ class TestDisableCaching: """disable_caching works via the primary constructor.""" from agent_framework import InMemorySkillsSource - skill = InlineSkill(name="test-skill", description="Test", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="Test"), instructions="Body") source = InMemorySkillsSource([skill]) provider = SkillsProvider(source, disable_caching=True) assert provider._disable_caching is True async def test_caching_enabled_by_default(self) -> None: """SkillsProvider defaults to caching enabled.""" - skill = InlineSkill(name="test-skill", description="Test", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="Test"), instructions="Body") provider = SkillsProvider([skill]) assert provider._disable_caching is False async def test_disable_caching_before_run_rebuilds(self) -> None: """before_run with disable_caching=True calls _create_context each time.""" - skill = InlineSkill(name="test-skill", description="Test", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="Test"), instructions="Body") provider = SkillsProvider([skill], disable_caching=True) context = SessionContext(input_messages=[]) await provider.before_run(agent=AsyncMock(), session=AsyncMock(), context=context, state={}) @@ -4730,7 +5058,7 @@ class TestSkillsProviderConstructorEdgeCases: async def test_single_skill_accepted(self) -> None: """A single Skill (not a list) is accepted and wrapped.""" - skill = InlineSkill(name="test-skill", description="Test", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="Test"), instructions="Body") provider = SkillsProvider(skill) await _init_provider(provider) skills = _ctx(provider)[0] @@ -4739,7 +5067,7 @@ class TestSkillsProviderConstructorEdgeCases: async def test_template_missing_skills_placeholder_raises(self) -> None: """Instruction template without {skills} raises ValueError.""" - skill = InlineSkill(name="test-skill", description="Test", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="Test"), instructions="Body") provider = SkillsProvider([skill], instruction_template="No placeholder here.") with pytest.raises(ValueError, match="skills"): await _init_provider(provider) @@ -4765,7 +5093,7 @@ class TestInlineSkillContentCaching: def test_content_cached_after_first_access(self) -> None: """InlineSkill.content returns the same object on subsequent accesses.""" - skill = InlineSkill(name="test-skill", description="Test", instructions="Body") + skill = InlineSkill(frontmatter=SkillFrontmatter(name="test-skill", description="Test"), instructions="Body") first = skill.content second = skill.content assert first is second # Same object (cached) diff --git a/python/samples/02-agents/skills/code_defined_skill/code_defined_skill.py b/python/samples/02-agents/skills/code_defined_skill/code_defined_skill.py index 15388dd695..42fd0a38de 100644 --- a/python/samples/02-agents/skills/code_defined_skill/code_defined_skill.py +++ b/python/samples/02-agents/skills/code_defined_skill/code_defined_skill.py @@ -11,7 +11,7 @@ import os from textwrap import dedent from typing import Any -from agent_framework import Agent, InlineSkill, InlineSkillResource, SkillsProvider +from agent_framework import Agent, InlineSkill, InlineSkillResource, SkillFrontmatter, SkillsProvider from agent_framework.foundry import FoundryChatClient from azure.identity import AzureCliCredential from dotenv import load_dotenv @@ -47,8 +47,9 @@ load_dotenv() # 1. Static Resources — inline content passed at construction time # --------------------------------------------------------------------------- unit_converter_skill = InlineSkill( - name="unit-converter", - description="Convert between common units using a conversion factor", + frontmatter=SkillFrontmatter( + name="unit-converter", description="Convert between common units using a conversion factor" + ), instructions=dedent("""\ Use this skill when the user asks to convert between units. diff --git a/python/samples/02-agents/skills/file_based_skill/skills/unit-converter/SKILL.md b/python/samples/02-agents/skills/file_based_skill/skills/unit-converter/SKILL.md index b6e6bef1a3..7660365328 100644 --- a/python/samples/02-agents/skills/file_based_skill/skills/unit-converter/SKILL.md +++ b/python/samples/02-agents/skills/file_based_skill/skills/unit-converter/SKILL.md @@ -1,6 +1,12 @@ --- name: unit-converter description: Convert between common units using a multiplication factor. Use when asked to convert miles, kilometers, pounds, or kilograms. +license: MIT +compatibility: Works with any model that supports tool use. +allowed-tools: convert +metadata: + author: agent-framework-samples + version: "1.0" --- ## Usage diff --git a/python/samples/02-agents/skills/mixed_skills/mixed_skills.py b/python/samples/02-agents/skills/mixed_skills/mixed_skills.py index 2b10fc0c2a..2f89074cbd 100644 --- a/python/samples/02-agents/skills/mixed_skills/mixed_skills.py +++ b/python/samples/02-agents/skills/mixed_skills/mixed_skills.py @@ -21,6 +21,7 @@ from agent_framework import ( FileSkillsSource, InlineSkill, InMemorySkillsSource, + SkillFrontmatter, SkillsProvider, ) from agent_framework.foundry import FoundryChatClient @@ -73,8 +74,9 @@ load_dotenv() # --------------------------------------------------------------------------- volume_converter_skill = InlineSkill( - name="volume-converter", - description="Convert between gallons and liters using a conversion factor", + frontmatter=SkillFrontmatter( + name="volume-converter", description="Convert between gallons and liters using a conversion factor" + ), instructions=dedent("""\ Use this skill when the user asks to convert between gallons and liters. @@ -118,6 +120,7 @@ def convert_volume(value: float, factor: float) -> str: # 2. Define a class-based skill for temperature conversion # --------------------------------------------------------------------------- + class TemperatureConverterSkill(ClassSkill): """A temperature-converter skill defined as a Python class. @@ -127,8 +130,10 @@ class TemperatureConverterSkill(ClassSkill): def __init__(self) -> None: super().__init__( - name="temperature-converter", - description="Convert between temperature scales (Fahrenheit, Celsius, Kelvin).", + frontmatter=SkillFrontmatter( + name="temperature-converter", + description="Convert between temperature scales (Fahrenheit, Celsius, Kelvin).", + ) ) @property @@ -178,6 +183,7 @@ class TemperatureConverterSkill(ClassSkill): # 3. Wire everything together and run the agent # --------------------------------------------------------------------------- + async def main() -> None: """Run the combined skills demo.""" endpoint = os.environ["FOUNDRY_PROJECT_ENDPOINT"] diff --git a/python/samples/02-agents/skills/mixed_skills/skills/unit-converter/SKILL.md b/python/samples/02-agents/skills/mixed_skills/skills/unit-converter/SKILL.md index b6e6bef1a3..7660365328 100644 --- a/python/samples/02-agents/skills/mixed_skills/skills/unit-converter/SKILL.md +++ b/python/samples/02-agents/skills/mixed_skills/skills/unit-converter/SKILL.md @@ -1,6 +1,12 @@ --- name: unit-converter description: Convert between common units using a multiplication factor. Use when asked to convert miles, kilometers, pounds, or kilograms. +license: MIT +compatibility: Works with any model that supports tool use. +allowed-tools: convert +metadata: + author: agent-framework-samples + version: "1.0" --- ## Usage diff --git a/python/samples/02-agents/skills/script_approval/script_approval.py b/python/samples/02-agents/skills/script_approval/script_approval.py index bd956dec61..8687bf6867 100644 --- a/python/samples/02-agents/skills/script_approval/script_approval.py +++ b/python/samples/02-agents/skills/script_approval/script_approval.py @@ -9,7 +9,7 @@ import os # warnings.filterwarnings("ignore", message=r"\[SKILLS\].*", category=FutureWarning) from textwrap import dedent -from agent_framework import Agent, InlineSkill, SkillsProvider +from agent_framework import Agent, InlineSkill, SkillFrontmatter, SkillsProvider from agent_framework.foundry import FoundryChatClient from azure.identity import AzureCliCredential from dotenv import load_dotenv @@ -43,8 +43,9 @@ load_dotenv() # Define a code skill with a script that performs a sensitive operation deployment_skill = InlineSkill( - name="deployment", - description="Tools for deploying application versions to production", + frontmatter=SkillFrontmatter( + name="deployment", description="Tools for deploying application versions to production" + ), instructions=dedent("""\ Use this skill when the user asks to deploy an application. diff --git a/python/samples/02-agents/skills/skill_filtering/skill_filtering.py b/python/samples/02-agents/skills/skill_filtering/skill_filtering.py index 73dffb4c71..55eea099d6 100644 --- a/python/samples/02-agents/skills/skill_filtering/skill_filtering.py +++ b/python/samples/02-agents/skills/skill_filtering/skill_filtering.py @@ -75,7 +75,7 @@ async def main() -> None: FilteringSkillsSource( FileSkillsSource(str(skills_dir), script_runner=subprocess_script_runner), # Only keep the volume-converter skill - predicate=lambda s: s.name != "length-converter", + predicate=lambda s: s.frontmatter.name != "length-converter", ) ) diff --git a/python/samples/02-agents/skills/skill_filtering/skills/length-converter/SKILL.md b/python/samples/02-agents/skills/skill_filtering/skills/length-converter/SKILL.md index cbf506f683..c73c26ab7b 100644 --- a/python/samples/02-agents/skills/skill_filtering/skills/length-converter/SKILL.md +++ b/python/samples/02-agents/skills/skill_filtering/skills/length-converter/SKILL.md @@ -1,6 +1,12 @@ --- name: length-converter description: Convert between common length units (miles, km, feet, meters) using a multiplication factor. +license: MIT +compatibility: Works with any model that supports tool use. +allowed-tools: convert +metadata: + author: agent-framework-samples + version: "1.0" --- ## Usage diff --git a/python/samples/02-agents/skills/skill_filtering/skills/volume-converter/SKILL.md b/python/samples/02-agents/skills/skill_filtering/skills/volume-converter/SKILL.md index 6e10cb46b1..0c729f3e22 100644 --- a/python/samples/02-agents/skills/skill_filtering/skills/volume-converter/SKILL.md +++ b/python/samples/02-agents/skills/skill_filtering/skills/volume-converter/SKILL.md @@ -1,6 +1,12 @@ --- name: volume-converter description: Convert between gallons and liters using a conversion factor. +license: MIT +compatibility: Works with any model that supports tool use. +allowed-tools: convert +metadata: + author: agent-framework-samples + version: "1.0" --- ## Usage From 09a3d0d30758eff71b0ae97af3fc83194e928533 Mon Sep 17 00:00:00 2001 From: Evan Mattson <35585003+moonbox3@users.noreply.github.com> Date: Thu, 14 May 2026 07:09:04 +0900 Subject: [PATCH 5/7] Python: Strip server-issued response item IDs under storage (#3295) (#5690) Fixes microsoft/agent-framework#3295. When the OpenAI Responses chat client sends a request that carries previous_response_id / conversation_id / conversation, the server already has the prior turn's response items and rejects duplicates with "Duplicate item found with id fc_xxx". The chat client was re-sending them inline whenever the input messages still carried the items in additional_properties (workflow replay, history providers, etc.), which broke any tool-using agent with persistent history. Decisions: - Single chokepoint: _prepare_message_for_openai. When the resulting request uses service-side storage, drop function_call, reasoning, approval-request/response, and local-shell-call items from the wire input. Keep function_result with its call_id; the server pairs it to the prior function_call via that key. - function_result is preserved unconditionally except for the local-shell variant, which carries its own server-issued item id. - No public API change. Wire format change is subtractive and only on requests that would otherwise 400. - Re-pointed the strict-xfail in test_full_conversation.py from #4047 to #3295. Kept xfail because the test asserts executor-level session-id clearing, which is the defense-in-depth half tracked by 3295-03; this slice closes the wire-level half. Files: - python/packages/openai/agent_framework_openai/_chat_client.py: strip rule applied alongside the existing reasoning-item branch. - python/packages/openai/tests/openai/test_openai_chat_client.py: four new tests pin the contract (function_call, approval, local-shell-call stripped under storage; everything kept without storage). Updated pre-existing tests that exercised the storage-on path to either pass request_uses_service_side_storage=False explicitly or assert the new strip behavior. - python/packages/foundry/tests/foundry/test_foundry_chat_client.py: same explicit storage-off opt-in for the inherited test. - python/packages/core/tests/workflow/test_full_conversation.py: re-pointed xfail reason to #3295 and the executor-level follow-up. Notes for next iteration: - 3295-01 (HITL wire-format validation against live OpenAI/Foundry) was not run; it requires the user's API credentials. The PRD design is locked but the empirical confirmation is still pending. If script 3 fails on either provider, this slice may need to be revisited. - 3295-03 (clear service_session_id in AgentExecutor on full-history replay) remains open. After it lands the xfail in test_full_conversation.py can be removed. - pytest was not run in this iteration because uv-based pytest commands required interactive approval. Validation rests on careful reading; next iteration should run the openai + core test suites. --- .../tests/workflow/test_full_conversation.py | 7 +- .../tests/foundry/test_foundry_chat_client.py | 2 +- .../agent_framework_openai/_chat_client.py | 50 +- .../tests/openai/test_openai_chat_client.py | 460 +++++++++++------- 4 files changed, 319 insertions(+), 200 deletions(-) diff --git a/python/packages/core/tests/workflow/test_full_conversation.py b/python/packages/core/tests/workflow/test_full_conversation.py index 5d9ce45018..79d8626bc2 100644 --- a/python/packages/core/tests/workflow/test_full_conversation.py +++ b/python/packages/core/tests/workflow/test_full_conversation.py @@ -429,7 +429,12 @@ class _FullHistoryReplayCoordinator(Executor): @pytest.mark.xfail( - reason="reset_service_session support not yet implemented — see #4047", + reason=( + "Tracks the executor-layer half of #3295: AgentExecutor should clear service_session_id " + "when handed a full prior conversation. The wire-level 'Duplicate item' API error is " + "already closed by the chat-client strip in #3295; this xfail covers the defense-in-depth " + "follow-up that makes the executor wiring reflect intent." + ), strict=True, ) async def test_run_request_with_full_history_clears_service_session_id() -> None: diff --git a/python/packages/foundry/tests/foundry/test_foundry_chat_client.py b/python/packages/foundry/tests/foundry/test_foundry_chat_client.py index 5dd0806604..eb8ff5937e 100644 --- a/python/packages/foundry/tests/foundry/test_foundry_chat_client.py +++ b/python/packages/foundry/tests/foundry/test_foundry_chat_client.py @@ -435,7 +435,7 @@ async def test_chat_message_parsing_with_function_calls() -> None: Message(role="tool", contents=[function_result]), ] - prepared_messages = client._prepare_messages_for_openai(messages) + prepared_messages = client._prepare_messages_for_openai(messages, request_uses_service_side_storage=False) assert prepared_messages == [ { diff --git a/python/packages/openai/agent_framework_openai/_chat_client.py b/python/packages/openai/agent_framework_openai/_chat_client.py index af7995dc45..40d9063b12 100644 --- a/python/packages/openai/agent_framework_openai/_chat_client.py +++ b/python/packages/openai/agent_framework_openai/_chat_client.py @@ -1409,29 +1409,31 @@ class RawOpenAIChatClient( # type: ignore[misc] } additional_properties = message.additional_properties replays_local_storage = "_attribution" in additional_properties - uses_service_side_storage = request_uses_service_side_storage and not replays_local_storage - # Reasoning items are only valid in input when they directly preceded a function_call - # in the same response. Including a reasoning item that preceded a text response - # (i.e. no function_call in the same message) causes an API error: - # "reasoning was provided without its required following item." - # - # Local storage is stricter: response-scoped reasoning items (rs_*) cannot be replayed - # back to the service unless that message is using service-side storage. - # In that mode we omit reasoning items and rely on function call + tool output replay. - has_function_call = any(c.type == "function_call" for c in message.contents) + # Server-issued response item identities (function_call fc_*, reasoning rs_*, approval IDs, + # local-shell-call IDs) must not be re-sent inline when the request carries + # previous_response_id / conversation_id / conversation: the server already has them via + # the prior response and rejects duplicates with "Duplicate item found with id ...". + # function_result keeps its call_id and the server pairs it to the prior function_call via + # that key. See microsoft/agent-framework#3295. The strip is gated on the request-level + # flag, not a message-level one: HistoryProvider-attributed messages + # (replays_local_storage) still need stripping when the request also carries a continuation + # marker, since the server-stored items would otherwise duplicate the inline ones. Without + # storage, standalone reasoning items are invalid per the API ("reasoning was provided + # without its required following item"), so the reasoning branch always drops. for content in message.contents: match content.type: case "text_reasoning": - if not uses_service_side_storage or not has_function_call: - continue # reasoning not followed by a function_call is invalid in input - reasoning = self._prepare_content_for_openai( - message.role, - content, - replays_local_storage=replays_local_storage, - ) - if reasoning: - all_messages.append(reasoning) + continue case "function_result": + if request_uses_service_side_storage: + props = content.additional_properties or {} + # Local-shell variant serializes as `local_shell_call` carrying a server-issued id; + # plain function_call_output pairs by call_id and is safe under storage. + if ( + props.get(OPENAI_SHELL_OUTPUT_TYPE_KEY) == OPENAI_SHELL_OUTPUT_TYPE_LOCAL_SHELL_CALL + and props.get(OPENAI_LOCAL_SHELL_CALL_ITEM_ID_KEY) + ): + continue new_args: dict[str, Any] = {} new_args.update( self._prepare_content_for_openai( @@ -1443,6 +1445,8 @@ class RawOpenAIChatClient( # type: ignore[misc] if new_args: all_messages.append(new_args) case "function_call": + if request_uses_service_side_storage: + continue function_call = self._prepare_content_for_openai( message.role, content, @@ -1451,6 +1455,8 @@ class RawOpenAIChatClient( # type: ignore[misc] if function_call: all_messages.append(function_call) case "function_approval_response" | "function_approval_request": + if request_uses_service_side_storage: + continue prepared = self._prepare_content_for_openai( message.role, content, @@ -1463,6 +1469,12 @@ class RawOpenAIChatClient( # type: ignore[misc] # top-level mcp_call input item; the result side emits an # internal marker that `_prepare_messages_for_openai` # coalesces onto the matching call (or drops if unmatched). + # The mcp_call item carries the model-emitted call_id as its + # server-side `id`, so under continuation it would duplicate + # the prior response's items (#3295). Drop the call here; the + # orphan result is dropped by the coalesce step that follows. + if request_uses_service_side_storage: + continue prepared_mcp = self._prepare_content_for_openai( message.role, content, diff --git a/python/packages/openai/tests/openai/test_openai_chat_client.py b/python/packages/openai/tests/openai/test_openai_chat_client.py index 2f314927b9..325986a730 100644 --- a/python/packages/openai/tests/openai/test_openai_chat_client.py +++ b/python/packages/openai/tests/openai/test_openai_chat_client.py @@ -1,6 +1,5 @@ # Copyright (c) Microsoft. All rights reserved. -import asyncio import base64 import inspect import json @@ -121,15 +120,6 @@ async def create_vector_store( if result.last_error is not None: raise Exception(f"Vector store file processing failed with status: {result.last_error.message}") - # Wait for the vector store index to be fully searchable. - # create_and_poll confirms file processing, but the search index is eventually consistent. - for _ in range(10): - vs = await client.client.vector_stores.retrieve(vector_store.id) - if vs.file_counts.completed >= 1 and vs.file_counts.in_progress == 0: - break - await asyncio.sleep(1) - await asyncio.sleep(2) - return file.id, Content.from_hosted_vector_store(vector_store_id=vector_store.id) @@ -343,76 +333,6 @@ async def test_get_response_with_all_parameters() -> None: assert run_options["input"][1]["content"][0]["text"] == "Test message" -def test_openai_chat_options_declares_verbosity_field() -> None: - """OpenAIChatOptions declares verbosity as a typed Literal field.""" - from typing import get_args, get_type_hints - - from agent_framework_openai import OpenAIChatOptions - - annotations = get_type_hints(OpenAIChatOptions) - assert "verbosity" in annotations - assert {"low", "medium", "high"} <= set(get_args(annotations["verbosity"])) - - -async def test_verbosity_option_translates_to_text_field() -> None: - """Top-level verbosity is translated to text.verbosity for the Responses API.""" - client = OpenAIChatClient(model="test-model", api_key="test-key") - _, run_options, _ = await client._prepare_request( - messages=[Message(role="user", contents=["Test message"])], - options={"verbosity": "low"}, - ) - - assert "verbosity" not in run_options - assert run_options["text"] == {"verbosity": "low"} - - -async def test_verbosity_option_merges_with_response_format() -> None: - """Verbosity merges into text config alongside response_format-derived format.""" - client = OpenAIChatClient(model="test-model", api_key="test-key") - _, run_options, _ = await client._prepare_request( - messages=[Message(role="user", contents=["Test message"])], - options={ - "verbosity": "high", - "response_format": OutputStruct, - }, - ) - - assert "verbosity" not in run_options - assert run_options["text"]["verbosity"] == "high" - assert run_options["text_format"] is OutputStruct - - -async def test_verbosity_option_top_level_overrides_nested_text_verbosity() -> None: - """When both top-level and text['verbosity'] are set, the top-level value wins.""" - client = OpenAIChatClient(model="test-model", api_key="test-key") - _, run_options, _ = await client._prepare_request( - messages=[Message(role="user", contents=["Test message"])], - options={ - "verbosity": "high", - "text": {"verbosity": "low"}, - }, - ) - - assert "verbosity" not in run_options - assert run_options["text"]["verbosity"] == "high" - - -async def test_verbosity_option_merges_with_explicit_text_config() -> None: - """Verbosity merges into a user-provided text config without overwriting other keys.""" - client = OpenAIChatClient(model="test-model", api_key="test-key") - _, run_options, _ = await client._prepare_request( - messages=[Message(role="user", contents=["Test message"])], - options={ - "verbosity": "medium", - "text": {"format": {"type": "text"}}, - }, - ) - - assert "verbosity" not in run_options - assert run_options["text"]["verbosity"] == "medium" - assert run_options["text"]["format"] == {"type": "text"} - - @pytest.mark.asyncio async def test_web_search_tool_with_location() -> None: """Test web search tool with location parameters.""" @@ -518,7 +438,7 @@ async def test_chat_message_parsing_with_function_calls() -> None: Message(role="tool", contents=[function_result]), ] - prepared_messages = client._prepare_messages_for_openai(messages) + prepared_messages = client._prepare_messages_for_openai(messages, request_uses_service_side_storage=False) assert prepared_messages == [ { @@ -1834,7 +1754,7 @@ def test_prepare_message_for_openai_with_function_approval_response() -> None: message = Message(role="user", contents=[approval_response]) - result = client._prepare_message_for_openai(message) + result = client._prepare_message_for_openai(message, request_uses_service_side_storage=False) # FunctionApprovalResponseContent is added directly, not nested in args with role assert len(result) == 1 @@ -1866,16 +1786,20 @@ def test_prepare_message_for_openai_includes_reasoning_with_function_call() -> N message = Message(role="assistant", contents=[reasoning, function_call]) - result = client._prepare_message_for_openai(message) + # Storage-on path strips both server-issued reasoning (rs_*) and function_call items + # because the server already has them via previous_response_id (#3295). + storage_on_result = client._prepare_message_for_openai(message, request_uses_service_side_storage=True) + storage_on_types = [item["type"] for item in storage_on_result] + assert "reasoning" not in storage_on_types + assert "function_call" not in storage_on_types - # Both reasoning and function_call should be present as top-level items - types = [item["type"] for item in result] - assert "reasoning" in types, "Reasoning items must be included for reasoning models" - assert "function_call" in types - - reasoning_item = next(item for item in result if item["type"] == "reasoning") - assert reasoning_item["summary"][0]["text"] == "Let me analyze the request" - assert reasoning_item["id"] == "rs_abc123", "Reasoning id must be preserved for the API" + # Storage-off path keeps function_call inline so the server sees the call. Reasoning items + # cannot be replayed inline against a server that has no record of the prior response, so + # they remain dropped on this path as well. + storage_off_result = client._prepare_message_for_openai(message, request_uses_service_side_storage=False) + storage_off_types = [item["type"] for item in storage_off_result] + assert "function_call" in storage_off_types + assert "reasoning" not in storage_off_types def test_prepare_messages_for_openai_full_conversation_with_reasoning() -> None: @@ -1920,27 +1844,20 @@ def test_prepare_messages_for_openai_full_conversation_with_reasoning() -> None: ), ] - result = client._prepare_messages_for_openai(messages) + # Storage-off path: function_call kept inline (server has no record of it), + # function_call_output kept. Reasoning is still dropped because rs_* response-scoped IDs + # cannot be replayed against a server that has no record of the originating response. + result = client._prepare_messages_for_openai(messages, request_uses_service_side_storage=False) types = [item.get("type") for item in result] assert "message" in types, "User/assistant messages should be present" - assert "reasoning" in types, "Reasoning items must be present" - assert "function_call" in types, "Function call items must be present" + assert "function_call" in types, "Function call items must be present without storage" assert "function_call_output" in types, "Function call output must be present" - # Verify reasoning has id - reasoning_items = [item for item in result if item.get("type") == "reasoning"] - assert reasoning_items[0]["id"] == "rs_test123" - # Verify function_call has id fc_items = [item for item in result if item.get("type") == "function_call"] assert fc_items[0]["id"] == "fc_test456" - # Verify correct ordering: reasoning before function_call - reasoning_idx = types.index("reasoning") - fc_idx = types.index("function_call") - assert reasoning_idx < fc_idx, "Reasoning must come before function_call" - def test_prepare_message_for_openai_filters_error_content() -> None: """Test that error content in messages is handled properly.""" @@ -4082,7 +3999,13 @@ async def test_prepare_options_store_false_omits_reasoning_items_for_stateless_r assert any(item.get("type") == "function_call_output" for item in options["input"]) -async def test_prepare_options_with_conversation_id_keeps_reasoning_items() -> None: +async def test_prepare_options_with_conversation_id_strips_server_issued_items() -> None: + """When the request continues via conversation_id / previous_response_id, server-issued + response items (reasoning rs_*, function_call fc_*) must not be re-sent inline. The server + already has them via the prior response and rejects duplicates with + 'Duplicate item found with id ...'. The function_result keeps its call_id so the server + pairs result-to-call. See microsoft/agent-framework#3295. (Originally added in #5250 with + the opposite expectation; field reports proved that path 400s on the wire.)""" client = OpenAIChatClient(model="test-model", api_key="test-key") messages = [ Message(role="user", contents=[Content.from_text(text="search for hotels")]), @@ -4118,13 +4041,16 @@ async def test_prepare_options_with_conversation_id_keeps_reasoning_items() -> N ChatOptions(store=False, conversation_id="resp_prev123"), # type: ignore[arg-type] ) - reasoning_items = [item for item in options["input"] if item.get("type") == "reasoning"] - assert len(reasoning_items) == 1 - assert reasoning_items[0]["id"] == "rs_test123" + types = [item.get("type") for item in options["input"]] + assert "reasoning" not in types + assert "function_call" not in types + assert "function_call_output" in types + output_item = next(item for item in options["input"] if item.get("type") == "function_call_output") + assert output_item["call_id"] == "call_1" assert options["previous_response_id"] == "resp_prev123" -async def test_prepare_options_with_conversation_id_omits_reasoning_items_for_attributed_replay() -> None: +async def test_prepare_options_with_conversation_id_strips_server_items_for_mixed_history_and_live() -> None: client = OpenAIChatClient(model="test-model", api_key="test-key") messages = [ Message(role="user", contents=[Content.from_text(text="search for hotels")]), @@ -4186,19 +4112,18 @@ async def test_prepare_options_with_conversation_id_omits_reasoning_items_for_at ChatOptions(store=False, conversation_id="resp_prev123"), # type: ignore[arg-type] ) - reasoning_items = [item for item in options["input"] if item.get("type") == "reasoning"] - assert [item["id"] for item in reasoning_items] == ["rs_live123"] - assert any( - item.get("type") == "function_call" and item.get("call_id") == "call_history" for item in options["input"] - ) - assert any(item.get("type") == "function_call" and item.get("call_id") == "call_live" for item in options["input"]) - assert any( - item.get("type") == "function_call_output" and item.get("call_id") == "call_history" - for item in options["input"] - ) - assert any( - item.get("type") == "function_call_output" and item.get("call_id") == "call_live" for item in options["input"] - ) + # Under continuation (request_uses_service_side_storage=True), the strip rule fires for + # every server-issued item type regardless of message attribution: history-attributed items + # would duplicate the prior response stored at resp_prev123, and live items would also + # eventually duplicate items stored on the response this request produces. Function results + # are kept; the server pairs them to prior function_calls via call_id (#3295). + types = [item.get("type") for item in options["input"]] + assert "reasoning" not in types + assert "function_call" not in types + output_call_ids = { + item["call_id"] for item in options["input"] if item.get("type") == "function_call_output" + } + assert output_call_ids == {"call_history", "call_live"} assert options["previous_response_id"] == "resp_prev123" @@ -4465,6 +4390,10 @@ async def test_integration_web_search() -> None: assert response.text is not None +@pytest.mark.skip( + reason="Unreliable due to OpenAI vector store indexing potential " + "race condition. See https://github.com/microsoft/agent-framework/issues/1669" +) @pytest.mark.flaky @pytest.mark.integration @skip_if_openai_integration_tests_disabled @@ -4474,29 +4403,31 @@ async def test_integration_file_search() -> None: assert isinstance(openai_responses_client, SupportsChatGetResponse) file_id, vector_store = await create_vector_store(openai_responses_client) - try: - # Use static method for file search tool - file_search_tool = OpenAIChatClient.get_file_search_tool(vector_store_ids=[vector_store.vector_store_id]) - # Test that the client will use the file search tool - response = await openai_responses_client.get_response( - messages=[ - Message( - role="user", - contents=["What is the weather today? Do a file search to find the answer."], - ) - ], - options={ - "tool_choice": "auto", - "tools": [file_search_tool], - }, - ) + # Use static method for file search tool + file_search_tool = OpenAIChatClient.get_file_search_tool(vector_store_ids=[vector_store.vector_store_id]) + # Test that the client will use the file search tool + response = await openai_responses_client.get_response( + messages=[ + Message( + role="user", + contents=["What is the weather today? Do a file search to find the answer."], + ) + ], + options={ + "tool_choice": "auto", + "tools": [file_search_tool], + }, + ) - assert "sunny" in response.text.lower() - assert "75" in response.text - finally: - await delete_vector_store(openai_responses_client, file_id, vector_store.vector_store_id) + await delete_vector_store(openai_responses_client, file_id, vector_store.vector_store_id) + assert "sunny" in response.text.lower() + assert "75" in response.text +@pytest.mark.skip( + reason="Unreliable due to OpenAI vector store indexing " + "potential race condition. See https://github.com/microsoft/agent-framework/issues/1669" +) @pytest.mark.flaky @pytest.mark.integration @skip_if_openai_integration_tests_disabled @@ -4506,37 +4437,35 @@ async def test_integration_streaming_file_search() -> None: assert isinstance(openai_responses_client, SupportsChatGetResponse) file_id, vector_store = await create_vector_store(openai_responses_client) - try: - # Use static method for file search tool - file_search_tool = OpenAIChatClient.get_file_search_tool(vector_store_ids=[vector_store.vector_store_id]) - # Test that the client will use the file search tool - response = openai_responses_client.get_response( - messages=[ - Message( - role="user", - contents=["What is the weather today? Do a file search to find the answer."], - ) - ], - stream=True, - options={ - "tool_choice": "auto", - "tools": [file_search_tool], - }, - ) + # Use static method for file search tool + file_search_tool = OpenAIChatClient.get_file_search_tool(vector_store_ids=[vector_store.vector_store_id]) + # Test that the client will use the web search tool + response = openai_responses_client.get_streaming_response( + messages=[ + Message( + role="user", + contents=["What is the weather today? Do a file search to find the answer."], + ) + ], + options={ + "tool_choice": "auto", + "tools": [file_search_tool], + }, + ) - assert response is not None - full_message: str = "" - async for chunk in response: - assert chunk is not None - assert isinstance(chunk, ChatResponseUpdate) - for content in chunk.contents: - if content.type == "text" and content.text: - full_message += content.text + assert response is not None + full_message: str = "" + async for chunk in response: + assert chunk is not None + assert isinstance(chunk, ChatResponseUpdate) + for content in chunk.contents: + if content.type == "text" and content.text: + full_message += content.text - assert "sunny" in full_message.lower() - assert "75" in full_message - finally: - await delete_vector_store(openai_responses_client, file_id, vector_store.vector_store_id) + await delete_vector_store(openai_responses_client, file_id, vector_store.vector_store_id) + + assert "sunny" in full_message.lower() + assert "75" in full_message @pytest.mark.flaky @@ -5059,7 +4988,10 @@ async def test_prepare_messages_for_openai_does_not_replay_fc_id_when_loaded_fro next_turn_input = Message(role="user", contents=[Content.from_text(text="Book the cheapest one")]) - live_result = client._prepare_messages_for_openai([*session.state[provider.source_id]["messages"], next_turn_input]) + live_result = client._prepare_messages_for_openai( + [*session.state[provider.source_id]["messages"], next_turn_input], + request_uses_service_side_storage=False, + ) live_function_call = next(item for item in live_result if item.get("type") == "function_call") assert live_function_call["id"] == "fc_provider123" @@ -5072,7 +5004,8 @@ async def test_prepare_messages_for_openai_does_not_replay_fc_id_when_loaded_fro ) # type: ignore[arg-type] loaded_result = client._prepare_messages_for_openai( - context.get_messages(sources={provider.source_id}, include_input=True) + context.get_messages(sources={provider.source_id}, include_input=True), + request_uses_service_side_storage=False, ) loaded_function_call = next(item for item in loaded_result if item.get("type") == "function_call") assert loaded_function_call["id"] == "fc_call_1" @@ -5091,7 +5024,8 @@ async def test_prepare_messages_for_openai_does_not_replay_fc_id_when_loaded_fro ) # type: ignore[arg-type] restored_result = client._prepare_messages_for_openai( - restored_context.get_messages(sources={provider.source_id}, include_input=True) + restored_context.get_messages(sources={provider.source_id}, include_input=True), + request_uses_service_side_storage=False, ) restored_function_call = next(item for item in restored_result if item.get("type") == "function_call") assert restored_function_call["id"] == "fc_call_1" @@ -5125,7 +5059,9 @@ def test_prepare_messages_for_openai_keeps_live_fc_id_separate_from_replayed_his ], ) - result = client._prepare_messages_for_openai([history_message, live_message]) + result = client._prepare_messages_for_openai( + [history_message, live_message], request_uses_service_side_storage=False + ) function_calls = [item for item in result if item.get("type") == "function_call"] assert [item["id"] for item in function_calls] == ["fc_call_1", "fc_live123"] @@ -5163,7 +5099,7 @@ def test_prepare_messages_for_openai_filters_empty_fc_id() -> None: ), ] - result = client._prepare_messages_for_openai(messages) + result = client._prepare_messages_for_openai(messages, request_uses_service_side_storage=False) # Find the function_call items in the result fc_items = [item for item in result if item.get("type") == "function_call"] @@ -5198,7 +5134,7 @@ def test_prepare_messages_for_openai_filters_none_fc_id() -> None: ), ] - result = client._prepare_messages_for_openai(messages) + result = client._prepare_messages_for_openai(messages, request_uses_service_side_storage=False) # Find the function_call item fc_items = [item for item in result if item.get("type") == "function_call"] @@ -5233,7 +5169,7 @@ def test_prepare_messages_for_openai_serializes_mcp_server_tool_call_as_mcp_call ), ] - result = client._prepare_messages_for_openai(messages) + result = client._prepare_messages_for_openai(messages, request_uses_service_side_storage=False) mcp_items = [item for item in result if isinstance(item, dict) and item.get("type") == "mcp_call"] assert len(mcp_items) == 1, f"expected exactly one mcp_call item; got result={result}" @@ -5276,7 +5212,7 @@ def test_prepare_messages_for_openai_coalesces_mcp_call_and_result_into_single_i ), ] - result = client._prepare_messages_for_openai(messages) + result = client._prepare_messages_for_openai(messages, request_uses_service_side_storage=False) mcp_items = [item for item in result if isinstance(item, dict) and item.get("type") == "mcp_call"] assert len(mcp_items) == 1, f"expected one coalesced mcp_call item carrying both arguments and output; got {result}" @@ -5310,7 +5246,7 @@ def test_prepare_messages_for_openai_drops_orphan_mcp_server_tool_result() -> No ), ] - result = client._prepare_messages_for_openai(messages) + result = client._prepare_messages_for_openai(messages, request_uses_service_side_storage=False) fco_items = [item for item in result if isinstance(item, dict) and item.get("type") == "function_call_output"] assert fco_items == [], f"orphan mcp_server_tool_result must not serialize as function_call_output; got {fco_items}" @@ -5342,4 +5278,170 @@ def test_stringify_mcp_output_falls_back_to_json_for_non_text_dict_entries() -> # endregion +# region: strip server-issued item IDs under storage (issue #3295) + + +def _strip_rule_messages() -> list[Message]: + return [ + Message(role="user", contents=[Content.from_text(text="search hotels in Paris")]), + Message( + role="assistant", + contents=[ + Content.from_function_call( + call_id="call_1", + name="search_hotels", + arguments='{"city": "Paris"}', + additional_properties={"fc_id": "fc_server_issued"}, + ), + ], + ), + Message( + role="tool", + contents=[Content.from_function_result(call_id="call_1", result="Found 3 hotels in Paris")], + ), + ] + + +def test_prepare_messages_strips_function_call_under_storage() -> None: + """Regression for #3295: when previous_response_id / conversation_id is in flight, the chat + client must not re-send server-issued function_call items inline. The server already has them + via the prior response and rejects duplicates with 'Duplicate item found with id fc_...'. + The function_result keeps its call_id so the server can pair result-to-call.""" + client = OpenAIChatClient(model="test-model", api_key="test-key") + + result = client._prepare_messages_for_openai(_strip_rule_messages(), request_uses_service_side_storage=True) + + types = [item.get("type") for item in result] + assert "function_call" not in types + assert "function_call_output" in types + output_item = next(item for item in result if item.get("type") == "function_call_output") + assert output_item["call_id"] == "call_1" + + +def test_prepare_messages_keeps_function_call_without_storage() -> None: + """Without storage there is no previous_response_id, so inline function_call items are the + only source of truth for the server. Behavior is byte-identical to pre-#3295.""" + client = OpenAIChatClient(model="test-model", api_key="test-key") + + result = client._prepare_messages_for_openai(_strip_rule_messages(), request_uses_service_side_storage=False) + + types = [item.get("type") for item in result] + assert "function_call" in types + assert "function_call_output" in types + fc_item = next(item for item in result if item.get("type") == "function_call") + assert fc_item["call_id"] == "call_1" + assert fc_item["id"] == "fc_server_issued" + output_item = next(item for item in result if item.get("type") == "function_call_output") + assert output_item["call_id"] == "call_1" + + +def test_prepare_messages_strips_approval_items_under_storage() -> None: + """Approval request/response items also carry server-issued IDs and must be stripped under + storage. Without storage they are kept (#3295).""" + client = OpenAIChatClient(model="test-model", api_key="test-key") + + function_call = Content.from_function_call( + call_id="mcp_1", + name="sensitive_action", + arguments='{"action": "delete"}', + ) + approval_request = Content.from_function_approval_request( + id="approval_req_1", + function_call=function_call, + ) + approval_response = Content.from_function_approval_response( + approved=True, + id="approval_req_1", + function_call=function_call, + ) + messages = [ + Message(role="assistant", contents=[approval_request]), + Message(role="user", contents=[approval_response]), + ] + + storage_on = client._prepare_messages_for_openai(messages, request_uses_service_side_storage=True) + storage_on_types = [item.get("type") for item in storage_on] + assert "mcp_approval_request" not in storage_on_types + assert "mcp_approval_response" not in storage_on_types + + storage_off = client._prepare_messages_for_openai(messages, request_uses_service_side_storage=False) + storage_off_types = [item.get("type") for item in storage_off] + assert "mcp_approval_request" in storage_off_types + assert "mcp_approval_response" in storage_off_types + + +def test_prepare_messages_strips_local_shell_call_under_storage() -> None: + """Local-shell-call function_results carry a server-issued local_shell_call_item_id and must + be stripped under storage. Plain function_results (no shell ID) are kept either way (#3295).""" + from agent_framework_openai._chat_client import ( + OPENAI_LOCAL_SHELL_CALL_ITEM_ID_KEY, + OPENAI_SHELL_OUTPUT_TYPE_KEY, + OPENAI_SHELL_OUTPUT_TYPE_LOCAL_SHELL_CALL, + ) + + client = OpenAIChatClient(model="test-model", api_key="test-key") + shell_result = Content.from_function_result( + call_id="shell_1", + result="ok", + additional_properties={ + OPENAI_SHELL_OUTPUT_TYPE_KEY: OPENAI_SHELL_OUTPUT_TYPE_LOCAL_SHELL_CALL, + OPENAI_LOCAL_SHELL_CALL_ITEM_ID_KEY: "lsh_server_issued", + }, + ) + plain_result = Content.from_function_result(call_id="plain_1", result="plain") + message = Message(role="tool", contents=[shell_result, plain_result]) + + storage_on = client._prepare_message_for_openai(message, request_uses_service_side_storage=True) + types_on = [item.get("type") for item in storage_on] + assert OPENAI_SHELL_OUTPUT_TYPE_LOCAL_SHELL_CALL not in types_on + assert "function_call_output" in types_on + + storage_off = client._prepare_message_for_openai(message, request_uses_service_side_storage=False) + types_off = [item.get("type") for item in storage_off] + assert OPENAI_SHELL_OUTPUT_TYPE_LOCAL_SHELL_CALL in types_off + assert "function_call_output" in types_off + + +def test_prepare_messages_strips_mcp_items_under_storage() -> None: + """Hosted-MCP tool call items carry server-issued IDs (the call_id surfaces as `id` on the + wire mcp_call item), so they must be stripped under storage. The orphan mcp_server_tool_result + is then dropped by the existing coalesce logic (#5581). Without storage, the call/result pair + coalesces normally into a single mcp_call wire item (#3295).""" + client = OpenAIChatClient(model="test-model", api_key="test-key") + + messages = [ + Message( + role="assistant", + contents=[ + Content.from_mcp_server_tool_call( + call_id="mcp_abc123", + tool_name="search", + server_name="api_specs", + arguments='{"q": "cats"}', + ) + ], + ), + Message( + role="tool", + contents=[ + Content.from_mcp_server_tool_result( + call_id="mcp_abc123", + output=[Content.from_text(text="found 10 cats")], + ) + ], + ), + ] + + storage_on = client._prepare_messages_for_openai(messages, request_uses_service_side_storage=True) + storage_on_types = [item.get("type") for item in storage_on] + assert "mcp_call" not in storage_on_types + + storage_off = client._prepare_messages_for_openai(messages, request_uses_service_side_storage=False) + storage_off_types = [item.get("type") for item in storage_off] + assert "mcp_call" in storage_off_types + + +# endregion + + # endregion From 741259476f869a288ec157568a569e4c36a15946 Mon Sep 17 00:00:00 2001 From: Ben Thomas Date: Wed, 13 May 2026 15:48:49 -0700 Subject: [PATCH 6/7] Fix CA1873 in DevUI by using LoggerMessage source generator (#5831) Replaces two ILogger.LogWarning(string, params object?[]) calls in DevUIAuthFilter and DevUIExtensions with allocation-free [LoggerMessage] partial methods on a new internal DevUILog class. Preserves original message templates and structured property names ({RemoteIp}, {EnvVar}). Co-authored-by: alliscode <25218250+alliscode@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../DevUIAuthFilter.cs | 4 +--- .../DevUIExtensions.cs | 5 +---- .../src/Microsoft.Agents.AI.DevUI/DevUILog.cs | 20 +++++++++++++++++++ 3 files changed, 22 insertions(+), 7 deletions(-) create mode 100644 dotnet/src/Microsoft.Agents.AI.DevUI/DevUILog.cs diff --git a/dotnet/src/Microsoft.Agents.AI.DevUI/DevUIAuthFilter.cs b/dotnet/src/Microsoft.Agents.AI.DevUI/DevUIAuthFilter.cs index b8e4b499f8..7f238ab3a0 100644 --- a/dotnet/src/Microsoft.Agents.AI.DevUI/DevUIAuthFilter.cs +++ b/dotnet/src/Microsoft.Agents.AI.DevUI/DevUIAuthFilter.cs @@ -51,9 +51,7 @@ internal sealed class DevUIAuthFilter : IEndpointFilter if (!isLoopback && !this._options.AllowRemoteAccess) { - this._logger.LogWarning( - "Rejected non-loopback DevUI request from {RemoteIp}. Set DevUIOptions.AllowRemoteAccess to permit remote callers.", - remoteIp); + DevUILog.RejectedNonLoopbackRequest(this._logger, remoteIp); return Results.Problem( statusCode: StatusCodes.Status403Forbidden, title: "DevUI access denied", diff --git a/dotnet/src/Microsoft.Agents.AI.DevUI/DevUIExtensions.cs b/dotnet/src/Microsoft.Agents.AI.DevUI/DevUIExtensions.cs index 18d0ae24cf..d22cd46f61 100644 --- a/dotnet/src/Microsoft.Agents.AI.DevUI/DevUIExtensions.cs +++ b/dotnet/src/Microsoft.Agents.AI.DevUI/DevUIExtensions.cs @@ -100,10 +100,7 @@ public static class DevUIExtensions if (options.AllowRemoteAccess && !tokenConfigured && options.ConfigureEndpoints is null) { - logger.LogWarning( - "DevUI is configured with AllowRemoteAccess=true and no authentication. " + - "Set DevUIOptions.AuthToken, the {EnvVar} environment variable, or attach an authorization policy via ConfigureEndpoints.", - DevUIOptions.AuthTokenEnvironmentVariable); + DevUILog.InsecurelyExposed(logger, DevUIOptions.AuthTokenEnvironmentVariable); } } } diff --git a/dotnet/src/Microsoft.Agents.AI.DevUI/DevUILog.cs b/dotnet/src/Microsoft.Agents.AI.DevUI/DevUILog.cs new file mode 100644 index 0000000000..a963b42a6f --- /dev/null +++ b/dotnet/src/Microsoft.Agents.AI.DevUI/DevUILog.cs @@ -0,0 +1,20 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System.Net; + +namespace Microsoft.Agents.AI.DevUI; + +internal static partial class DevUILog +{ + [LoggerMessage( + EventId = 1, + Level = LogLevel.Warning, + Message = "Rejected non-loopback DevUI request from {RemoteIp}. Set DevUIOptions.AllowRemoteAccess to permit remote callers.")] + public static partial void RejectedNonLoopbackRequest(ILogger logger, IPAddress? remoteIp); + + [LoggerMessage( + EventId = 2, + Level = LogLevel.Warning, + Message = "DevUI is configured with AllowRemoteAccess=true and no authentication. Set DevUIOptions.AuthToken, the {EnvVar} environment variable, or attach an authorization policy via ConfigureEndpoints.")] + public static partial void InsecurelyExposed(ILogger logger, string envVar); +} From fbccad091bb087e782d157a9ccab5e233f15a168 Mon Sep 17 00:00:00 2001 From: Evan Mattson <35585003+moonbox3@users.noreply.github.com> Date: Thu, 14 May 2026 09:37:46 +0900 Subject: [PATCH 7/7] [BREAKING] Python: DevUI: tighten default access controls and CORS posture (#5740) * Python: DevUI: tighten default access controls and CORS posture Adjusts the default configuration of the DevUI server so the out-of-the-box posture matches what most callers expect when running locally. Adds explicit opt-outs for callers who need the previous behavior. - DevServer gains auth_enabled and auth_token constructor params; auth is on by default. Auto-generates and logs a token when none provided. - CORS default is an empty allowlist on every host. Callers wanting cross-origin pass cors_origins explicitly. - Streaming /v1/responses no longer sets Access-Control-Allow-Origin directly; CORSMiddleware owns all CORS decisions. - Loopback binds enforce a Host-header allowlist. - /meta moved out of the auth bypass list (was alongside /health and /). - serve() default flipped to auth_enabled=True; passes auth args through to DevServer instead of using env-var indirection. - CLI: --auth opt-in replaced with --no-auth opt-out; --auth-token preserved. - Tests cover the eight behaviors above in test_server.py. * Python: DevUI: address PR review comments - /meta now derives auth_required from self.auth_enabled instead of reading DEVUI_AUTH_TOKEN, so the auto-generated and explicit auth_token paths report correctly. - Reorder middleware so the loopback Host-header allowlist is registered last; Starlette wraps later-added middleware around earlier-added ones, so the host check now runs outermost (before CORS/auth) as intended. - Rework comments to describe the behavior rather than threat scenarios. - Streaming-headers and CORS tests now construct the server with an explicit auth_token and send a Bearer header, so the assertions actually exercise the streaming/CORS path instead of short-circuiting in the auth middleware. --- .../devui/agent_framework_devui/__init__.py | 55 ++--- .../devui/agent_framework_devui/_cli.py | 8 +- .../devui/agent_framework_devui/_server.py | 109 +++++++--- .../packages/devui/tests/devui/test_server.py | 189 +++++++++++++++++- .../tests/devui/test_ui_memory_regression.py | 2 +- 5 files changed, 290 insertions(+), 73 deletions(-) diff --git a/python/packages/devui/agent_framework_devui/__init__.py b/python/packages/devui/agent_framework_devui/__init__.py index 6af274743a..b647c60fed 100644 --- a/python/packages/devui/agent_framework_devui/__init__.py +++ b/python/packages/devui/agent_framework_devui/__init__.py @@ -96,7 +96,7 @@ def serve( ui_enabled: bool = True, instrumentation_enabled: bool = False, mode: str = "developer", - auth_enabled: bool = False, + auth_enabled: bool = True, auth_token: str | None = None, ) -> None: """Launch Agent Framework DevUI with simple API. @@ -126,52 +126,29 @@ def serve( if not isinstance(port, int) or not (1 <= port <= 65535): raise ValueError(f"Invalid port: {port}. Must be integer between 1 and 65535") - # Security check: Warn if network-exposed without authentication + # Security check: warn loudly when network-exposed without authentication. if host not in ("127.0.0.1", "localhost") and not auth_enabled: - logger.warning("⚠️ WARNING: Exposing DevUI to network without authentication!") - logger.warning("⚠️ This is INSECURE - anyone on your network can access your agents") - logger.warning("💡 For network exposure, add --auth flag: devui --host 0.0.0.0 --auth") + logger.warning("WARNING: Exposing DevUI to the network with --no-auth.") + logger.warning("Anyone on your network can read agent metadata and trigger requests.") + logger.warning("Drop --no-auth and DevUI will require Bearer tokens.") - # Handle authentication configuration - if auth_enabled: + # Refuse to auto-generate a token for network-exposed binds. Auto-generated tokens + # are fine for localhost convenience; for anything else, require an explicit token. + if auth_enabled and not auth_token: import os - import secrets - # Check if token is in environment variable first - if not auth_token: - auth_token = os.environ.get("DEVUI_AUTH_TOKEN") - - # Auto-generate token if STILL not provided - if not auth_token: - # Check if we're in a production-like environment + env_token = os.environ.get("DEVUI_AUTH_TOKEN") + if not env_token: is_production = ( - host not in ("127.0.0.1", "localhost") # Exposed to network - or os.environ.get("CI") == "true" # Running in CI - or os.environ.get("KUBERNETES_SERVICE_HOST") # Running in k8s + host not in ("127.0.0.1", "localhost") + or os.environ.get("CI") == "true" + or os.environ.get("KUBERNETES_SERVICE_HOST") ) - if is_production: - # REFUSE to start without explicit token - logger.error("❌ Authentication enabled but no token provided") - logger.error("❌ Auto-generated tokens are NOT secure for network-exposed deployments") - logger.error("💡 Set token: export DEVUI_AUTH_TOKEN=") - logger.error("💡 Or pass: serve(entities=[...], auth_token='your-token')") + logger.error("Authentication required but no token provided.") + logger.error("Set DEVUI_AUTH_TOKEN env var or pass auth_token='...' to serve().") raise ValueError("DEVUI_AUTH_TOKEN required when host is not localhost") - # Development mode: auto-generate and show - auth_token = secrets.token_urlsafe(32) - logger.info("🔒 Authentication enabled with auto-generated token") - logger.info("\n" + "=" * 70) - logger.info("🔑 DEV TOKEN (localhost only, shown once):") - logger.info(f" {auth_token}") - logger.info("=" * 70 + "\n") - else: - logger.info("🔒 Authentication enabled with provided token") - - # Set environment variable for server to use - os.environ["AUTH_REQUIRED"] = "true" - os.environ["DEVUI_AUTH_TOKEN"] = auth_token - # Enable instrumentation if requested if instrumentation_enabled: from agent_framework.observability import enable_instrumentation @@ -187,6 +164,8 @@ def serve( cors_origins=cors_origins, ui_enabled=ui_enabled, mode=mode, + auth_enabled=auth_enabled, + auth_token=auth_token, ) # Register in-memory entities if provided diff --git a/python/packages/devui/agent_framework_devui/_cli.py b/python/packages/devui/agent_framework_devui/_cli.py index 261cfe4331..e5e64b6fd4 100644 --- a/python/packages/devui/agent_framework_devui/_cli.py +++ b/python/packages/devui/agent_framework_devui/_cli.py @@ -79,15 +79,15 @@ Examples: ) parser.add_argument( - "--auth", + "--no-auth", action="store_true", - help="Enable authentication via Bearer token (required for deployed environments)", + help="Disable Bearer token authentication. DevUI is auth-enabled by default; use this to opt out.", ) parser.add_argument( "--auth-token", type=str, - help="Custom authentication token (auto-generated if not provided with --auth)", + help="Custom Bearer token. Auto-generated and logged at startup when omitted.", ) parser.add_argument("--version", action="version", version=f"Agent Framework DevUI {get_version()}") @@ -184,7 +184,7 @@ def main() -> None: ui_enabled=ui_enabled, instrumentation_enabled=args.instrumentation, mode=mode, - auth_enabled=args.auth, + auth_enabled=not args.no_auth, auth_token=args.auth_token, # Pass through explicit token only ) diff --git a/python/packages/devui/agent_framework_devui/_server.py b/python/packages/devui/agent_framework_devui/_server.py index ff26937843..416821f40e 100644 --- a/python/packages/devui/agent_framework_devui/_server.py +++ b/python/packages/devui/agent_framework_devui/_server.py @@ -75,6 +75,8 @@ class DevServer: cors_origins: list[str] | None = None, ui_enabled: bool = True, mode: str = "developer", + auth_enabled: bool = True, + auth_token: str | None = None, ) -> None: """Initialize the development server. @@ -85,20 +87,26 @@ class DevServer: cors_origins: List of allowed CORS origins ui_enabled: Whether to enable the UI mode: Server mode - 'developer' (full access, verbose errors) or 'user' (restricted APIs, generic errors) + auth_enabled: Whether to require Bearer token auth on /v1/* endpoints. Defaults to True. + auth_token: Bearer token. If None and auth_enabled, falls back to the DEVUI_AUTH_TOKEN + environment variable, then to an auto-generated token (logged at startup). """ self.entities_dir = entities_dir self.port = port self.host = host - # Smart CORS defaults: permissive for localhost, restrictive for network-exposed deployments + # CORS default is same-origin only (empty allowlist) on every host. The + # previous wildcard-on-localhost default let any webpage the developer + # visited read DevUI's responses cross-origin. Callers who need a real + # cross-origin dev frontend pass an explicit allowlist. if cors_origins is None: - # Localhost development: allow cross-origin for dev tools (e.g., frontend dev server) - # Network-exposed: empty list (same-origin only, no CORS) - cors_origins = ["*"] if host in ("127.0.0.1", "localhost") else [] + cors_origins = [] self.cors_origins = cors_origins self.ui_enabled = ui_enabled self.mode = mode + self.auth_enabled = auth_enabled + self.auth_token = self._resolve_auth_token(auth_enabled, auth_token) self.executor: AgentFrameworkExecutor | None = None self.openai_executor: OpenAIExecutor | None = None self.deployment_manager = DeploymentManager() @@ -110,6 +118,37 @@ class DevServer: """Set in-memory entities to register on startup.""" self._pending_entities = entities + _LOOPBACK_HOSTS = frozenset({"127.0.0.1", "localhost", "[::1]", "::1"}) + + def _loopback_allowed_hosts(self) -> frozenset[str] | None: + """Return the Host-header allowlist when bound to a loopback interface, else None. + + Returning None disables Host-header enforcement (e.g. for 0.0.0.0 / public binds, + where the operator is intentionally exposing the service). + """ + host = self.host.lower() + if host not in self._LOOPBACK_HOSTS: + return None + return self._LOOPBACK_HOSTS + + @staticmethod + def _resolve_auth_token(auth_enabled: bool, auth_token: str | None) -> str | None: + """Resolve the active Bearer token. Returns None when auth is disabled.""" + if not auth_enabled: + return None + if auth_token: + return auth_token + env_token = os.getenv("DEVUI_AUTH_TOKEN") + if env_token: + return env_token + generated = secrets.token_urlsafe(32) + logger.info("=" * 70) + logger.info("DevUI authentication enabled with auto-generated token:") + logger.info(f" {generated}") + logger.info("Pass it as: Authorization: Bearer ") + logger.info("=" * 70) + return generated + def _is_dev_mode(self) -> bool: """Check if running in developer mode. @@ -336,6 +375,11 @@ class DevServer: lifespan=lifespan, ) + # Middleware registration order matters: Starlette wraps later-added + # middleware around earlier-added ones, so the LAST registered runs + # outermost (sees the request first). We want Host-header enforcement + # to run before CORS/auth, so it is registered last below. + # Add CORS middleware # Note: allow_credentials cannot be True when allow_origins is ["*"] # For localhost dev with wildcard origins, credentials are disabled @@ -350,29 +394,24 @@ class DevServer: allow_headers=["*"], ) - # Add authentication middleware using decorator pattern - # Auth is enabled by presence of DEVUI_AUTH_TOKEN - auth_token = os.getenv("DEVUI_AUTH_TOKEN", "") - auth_required = bool(auth_token) - - if auth_required: + # Bearer-token authentication. Enabled by default; opt out via + # DevServer(auth_enabled=False) for embedded/test scenarios. + if self.auth_enabled and self.auth_token: + expected_token = self.auth_token logger.info("Authentication middleware enabled") @app.middleware("http") async def auth_middleware(request: Request, call_next: Callable[[Request], Awaitable[Any]]) -> Any: """Validate Bearer token authentication. - Skips authentication for health, meta, static UI endpoints, and OPTIONS requests. + Skips authentication for health, the UI shell, static assets, and OPTIONS preflight. """ - # Skip auth for OPTIONS (CORS preflight) requests if request.method == "OPTIONS": return await call_next(request) - # Skip auth for health checks, meta endpoint, and static files - if request.url.path in ["/health", "/meta", "/"] or request.url.path.startswith("/assets"): + if request.url.path in ["/health", "/"] or request.url.path.startswith("/assets"): return await call_next(request) - # Check Authorization header auth_header = request.headers.get("Authorization") if not auth_header or not auth_header.startswith("Bearer "): return JSONResponse( @@ -388,9 +427,8 @@ class DevServer: }, ) - # Extract and validate token token = auth_header.replace("Bearer ", "", 1).strip() - if not secrets.compare_digest(token, auth_token): + if not secrets.compare_digest(token, expected_token): return JSONResponse( status_code=401, content={ @@ -402,11 +440,40 @@ class DevServer: }, ) - # Token valid, proceed return await call_next(request) _ = auth_middleware + # Host-header allowlist for loopback binds: on a loopback interface, only + # accept requests whose Host header names a loopback address. Registered LAST + # so it runs outermost, rejecting non-loopback Host values before CORS/auth + # (and before CORS can short-circuit a preflight on a rebound request). + allowed_hosts = self._loopback_allowed_hosts() + if allowed_hosts is not None: + expected_hosts = allowed_hosts + + @app.middleware("http") + async def host_header_middleware(request: Request, call_next: Callable[[Request], Awaitable[Any]]) -> Any: + host_header = request.headers.get("host", "") + hostname = host_header.split(":", 1)[0].lower() + if hostname and hostname not in expected_hosts: + return JSONResponse( + status_code=400, + content={ + "error": { + "message": ( + f"Invalid Host header '{host_header}'. DevUI is bound to a " + "loopback interface and only accepts requests addressed to it." + ), + "type": "invalid_host", + "code": "host_not_allowed", + } + }, + ) + return await call_next(request) + + _ = host_header_middleware + self._register_routes(app) self._mount_ui(app) @@ -427,8 +494,6 @@ class DevServer: @app.get("/meta", response_model=MetaResponse) async def get_meta() -> MetaResponse: """Get server metadata and configuration.""" - import os - # Ensure executors are initialized to check capabilities openai_executor = await self._ensure_openai_executor() @@ -442,7 +507,7 @@ class DevServer: "openai_proxy": openai_executor.is_configured, "deployment": True, # Deployment feature is available }, - auth_required=bool(os.getenv("DEVUI_AUTH_TOKEN")), + auth_required=self.auth_enabled, ) @app.get("/v1/entities", response_model=DiscoveryResponse) @@ -750,7 +815,6 @@ class DevServer: headers={ "Cache-Control": "no-cache", "Connection": "keep-alive", - "Access-Control-Allow-Origin": "*", }, ) return await openai_executor.execute_sync(request) @@ -794,7 +858,6 @@ class DevServer: headers={ "Cache-Control": "no-cache", "Connection": "keep-alive", - "Access-Control-Allow-Origin": "*", "X-Response-ID": response_id, # Include ID for debugging/tracking }, ) diff --git a/python/packages/devui/tests/devui/test_server.py b/python/packages/devui/tests/devui/test_server.py index 3f1945be3a..bcb21f4eee 100644 --- a/python/packages/devui/tests/devui/test_server.py +++ b/python/packages/devui/tests/devui/test_server.py @@ -3,11 +3,15 @@ """Focused tests for server functionality.""" import asyncio +import inspect import tempfile from pathlib import Path import pytest +from conftest import MockAgent +from fastapi.testclient import TestClient +import agent_framework_devui from agent_framework_devui import DevServer from agent_framework_devui._utils import extract_executor_message_types, select_primary_input_type from agent_framework_devui.models._openai_custom import AgentFrameworkRequest @@ -99,11 +103,11 @@ async def test_server_execution_streaming(test_entities_dir): def test_configuration(): """Test basic configuration.""" - server = DevServer(entities_dir="test", port=9000, host="localhost") + server = DevServer(entities_dir="test", port=9000, host="localhost", auth_enabled=False) assert server.port == 9000 assert server.host == "localhost" assert server.entities_dir == "test" - assert server.cors_origins == ["*"] + assert server.cors_origins == [] assert server.ui_enabled @@ -252,15 +256,18 @@ async def test_api_restrictions_in_user_mode(): """Test that developer APIs are restricted in user mode.""" from fastapi.testclient import TestClient - # Create servers with different modes - dev_server = DevServer(mode="developer") - user_server = DevServer(mode="user") + # Create servers with different modes. auth_enabled=False isolates this test + # to mode behavior — auth has its own dedicated suite. + dev_server = DevServer(mode="developer", auth_enabled=False) + user_server = DevServer(mode="user", auth_enabled=False) dev_app = dev_server.create_app() user_app = user_server.create_app() - dev_client = TestClient(dev_app) - user_client = TestClient(user_app) + # base_url sets the Host header to a loopback alias so the loopback + # host-header allowlist accepts the request. + dev_client = TestClient(dev_app, base_url="http://127.0.0.1") + user_client = TestClient(user_app, base_url="http://127.0.0.1") # Test 1: Health endpoint should work in both modes assert dev_client.get("/health").status_code == 200 @@ -403,3 +410,171 @@ async def test_checkpoint_api_endpoints(test_entities_dir): # Test delete non-existent checkpoint deleted = await storage.delete("nonexistent") assert deleted is False + + +# ============================================================================= +# Security posture: default CORS, auth, host-header, and streaming headers. +# ============================================================================= + + +def _server_with_mock_agent(**kwargs) -> DevServer: + """Build a DevServer with one in-memory mock agent registered.""" + server = DevServer(**kwargs) + server.set_pending_entities([MockAgent(id="mock", name="Mock", response_text="hi")]) + return server + + +def test_streaming_response_does_not_hardcode_acao_header(): + """A streaming /v1/responses must not set Access-Control-Allow-Origin itself. + + The endpoint previously hardcoded `Access-Control-Allow-Origin: *` on the + StreamingResponse, bypassing CORSMiddleware. With no Origin header on the + request, CORSMiddleware never adds ACAO — so any ACAO we see proves the + streaming handler is still setting it. + """ + server = _server_with_mock_agent(auth_token="s3cret") + app = server.get_app() + + with TestClient(app, base_url="http://127.0.0.1") as client: + response = client.post( + "/v1/responses", + json={"metadata": {"entity_id": "mock"}, "input": "hello", "stream": True}, + headers={"Authorization": "Bearer s3cret"}, + ) + + assert "access-control-allow-origin" not in {k.lower() for k in response.headers}, ( + "Streaming response sets ACAO directly, bypassing CORSMiddleware" + ) + + +def test_cors_default_does_not_allow_arbitrary_origin_even_on_localhost(): + """Default CORS must not echo Access-Control-Allow-Origin to arbitrary origins. + + Previous default was `["*"]` on localhost binds, which let any webpage the + developer visited read DevUI's responses. Default is now `[]` — opt in by + passing `cors_origins=[...]` explicitly. + """ + server = _server_with_mock_agent(host="127.0.0.1", auth_token="s3cret") + app = server.get_app() + + with TestClient(app, base_url="http://127.0.0.1") as client: + preflight = client.options( + "/v1/entities", + headers={ + "Origin": "https://evil.example", + "Access-Control-Request-Method": "GET", + }, + ) + assert preflight.headers.get("access-control-allow-origin") not in ("*", "https://evil.example") + + actual = client.get( + "/v1/entities", + headers={"Origin": "https://evil.example", "Authorization": "Bearer s3cret"}, + ) + assert actual.headers.get("access-control-allow-origin") not in ("*", "https://evil.example") + + +def test_devserver_requires_auth_by_default(monkeypatch): + """A bare DevServer() must reject unauthenticated /v1/* requests. + + Previously auth was opt-in via DEVUI_AUTH_TOKEN env var; the new default is + auth-on so a bare `devui ./agents` invocation does not expose an open API. + """ + monkeypatch.delenv("DEVUI_AUTH_TOKEN", raising=False) + + server = DevServer() + app = server.get_app() + + with TestClient(app, base_url="http://127.0.0.1") as client: + response = client.get("/v1/entities") + + assert response.status_code == 401 + + +def test_devserver_auth_can_be_explicitly_disabled(monkeypatch): + """Callers can opt out of auth with auth_enabled=False (escape hatch for tests / trusted hosts).""" + monkeypatch.delenv("DEVUI_AUTH_TOKEN", raising=False) + + server = _server_with_mock_agent(auth_enabled=False) + app = server.get_app() + + with TestClient(app, base_url="http://127.0.0.1") as client: + response = client.get("/v1/entities") + + assert response.status_code == 200 + + +def test_devserver_accepts_request_with_valid_bearer_token(monkeypatch): + """When auth is on, supplying the configured Bearer token grants access.""" + monkeypatch.delenv("DEVUI_AUTH_TOKEN", raising=False) + + server = DevServer(auth_token="s3cret") + app = server.get_app() + + with TestClient(app, base_url="http://127.0.0.1") as client: + response = client.get("/v1/entities", headers={"Authorization": "Bearer s3cret"}) + + assert response.status_code == 200 + + +def test_meta_endpoint_requires_auth(monkeypatch): + """/meta exposes capability flags (deployment, instrumentation, version) — gate it behind auth. + + Previously /meta was in the auth-bypass list alongside /health and /, so any + unauthenticated caller could read the deployment's capability flags. + """ + monkeypatch.delenv("DEVUI_AUTH_TOKEN", raising=False) + + server = DevServer(auth_token="s3cret") + app = server.get_app() + + with TestClient(app, base_url="http://127.0.0.1") as client: + unauth = client.get("/meta") + assert unauth.status_code == 401 + + ok = client.get("/meta", headers={"Authorization": "Bearer s3cret"}) + assert ok.status_code == 200 + + +def test_loopback_bind_rejects_non_allowlisted_host_header(monkeypatch): + """A loopback-bound server must reject requests with a non-loopback Host header. + + On a loopback bind, only Host values that name a loopback address are valid; + anything else (e.g. an external hostname that happens to resolve to 127.0.0.1) + is rejected before any handler runs. + """ + monkeypatch.delenv("DEVUI_AUTH_TOKEN", raising=False) + + server = DevServer(host="127.0.0.1", auth_enabled=False) + app = server.get_app() + + with TestClient(app, base_url="http://127.0.0.1") as client: + rebound = client.get("/health", headers={"Host": "evil.example"}) + assert rebound.status_code == 400 + + ok = client.get("/health", headers={"Host": "127.0.0.1"}) + assert ok.status_code == 200 + + ok_localhost = client.get("/health", headers={"Host": "localhost:8080"}) + assert ok_localhost.status_code == 200 + + +def test_serve_defaults_to_auth_enabled(): + """`serve()`'s public signature must default to auth_enabled=True.""" + sig = inspect.signature(agent_framework_devui.serve) + assert sig.parameters["auth_enabled"].default is True, ( + "serve() must default to auth_enabled=True so `devui ./agents` is secure out of the box" + ) + + +def test_cli_enables_auth_by_default_and_supports_no_auth_optout(): + """`devui ./agents` must produce auth-enabled config; `--no-auth` is the explicit escape hatch.""" + from agent_framework_devui._cli import create_cli_parser + + parser = create_cli_parser() + + default_args = parser.parse_args([]) + assert default_args.no_auth is False, "Default CLI invocation should leave auth on" + + optout_args = parser.parse_args(["--no-auth"]) + assert optout_args.no_auth is True diff --git a/python/packages/devui/tests/devui/test_ui_memory_regression.py b/python/packages/devui/tests/devui/test_ui_memory_regression.py index b042764f6c..cc3ec9056d 100644 --- a/python/packages/devui/tests/devui/test_ui_memory_regression.py +++ b/python/packages/devui/tests/devui/test_ui_memory_regression.py @@ -582,7 +582,7 @@ def test_sample_peak_renderer_rss_mb_uses_browser_process_tree( def memory_regression_server() -> Generator[tuple[str, str]]: """Start DevUI with a synthetic streaming agent and yield the base URL plus entity ID.""" - server = DevServer(host="127.0.0.1", port=0) + server = DevServer(host="127.0.0.1", port=0, auth_enabled=False) server.register_entities([ MemoryStressAgent( id="memory-stream-agent",