diff --git a/dotnet/agent-framework-dotnet.slnx b/dotnet/agent-framework-dotnet.slnx index 24c04feb7f..ddb609beed 100644 --- a/dotnet/agent-framework-dotnet.slnx +++ b/dotnet/agent-framework-dotnet.slnx @@ -299,6 +299,7 @@ + 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 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.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); +} 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 // --------------------------------------------------------------- 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/_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/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_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") 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/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/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", 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 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