From 3540f733f1fc822ca35a36be043400ae4a2f8f84 Mon Sep 17 00:00:00 2001 From: "mars.yu" Date: Thu, 20 Aug 2026 21:39:42 +0800 Subject: [PATCH 1/3] Agent test harness: routing cases, authored history, scope narrowing, metrics Everything in one commit because no smaller one builds. AgentTestCaseRunner.cs and AgentTestingPlugin.cs each carry changes from several of the features below, and splitting by file leaves an intermediate that fails to compile -- the runner would reference an interface member that no longer exists, or the plugin would register a judge whose implementation is not yet present. That also folds in the llmJudge work that was already sitting uncommitted in this worktree. Routing and agent chains ------------------------ Cases now declare what they verify (CaseType: Routing or Agent) and where the conversation opens (EntryAgentId, defaulting to the suite own agent). That one value decides whether the router is part of what a case measures: ConversationService sends a routing-type agent through InstructLoop, which can hand off, and everything else through InstructDirect, which cannot. route_to_agent is on the mock allow list and so never reaches MockFunctionExecutor, the only caller that records an observed tool call -- routing decisions were therefore invisible. The chain is now reconstructed from the conversation own assistant dialogs, each of which carries the agent that wrote it, and reported per turn and per case with consecutive repeats collapsed by agent id. A new agentChain assertion compares that chain in three modes: contains, ordered (the hand-off assertion) and exact (which is how a case asserts nothing routed away). An unrecognised mode fails rather than falling back to the loosest check. Both agentChain and routedToAgent accept an agent id or its display name: the id is what an author copies out of the agent list, and asserting an id against a name could never pass no matter what the agent did. routedToAgent now reads the chain last entry instead of a separate field, so the two cannot disagree, and a turn-level assertion sees that turn own slice -- a turn that produced no answer no longer inherits the previous turn agent. Routing accuracy is tallied per model on the run, counting only Routing cases. Errored cases count against it: "could not tell" is not "routed correctly". E2E was dropped as a case type. A journey across several agents is an Agent case whose agentChain assertion describes the hand-offs, and a third type bought only a third branch in every validation and aggregation path. Authored history ---------------- A case can carry prior turns, written into the conversation before it runs, so a real exchange becomes a fixed starting context. Not driven through the model: no token cost, and the preamble cannot itself become a source of flakiness. AppendConversationDialogs is an UpdateOne with no upsert, so it silently writes nothing when the conversation dialog document does not exist -- and PrepareAsync deliberately does not create it. The conversation is therefore created first, through the same call SendMessage uses, and the write is read back and counted. A short count errors the case: running without the context it was written around would otherwise report an ordinary pass or fail about a scenario that never existed. Authored history is excluded from the agent chain. It is not something the agent did, and letting it in would fail an exact chain assertion for a reason the author never caused. Copying a case -------------- POST cases/{id}/copy duplicates a case inside its suite. Server-side because the copy has to carry every field: a client that rebuilds the payload from its own form drops what it does not know about, and a copy missing its mocks is indistinguishable in the list until the run where it blocks every tool. Cloned by a BSON round trip for the same reason -- a hand-written clone would silently omit the next field added. The copy lands disabled whatever the source was. An exact duplicate joining the next run measures the same thing twice, and for a routing case it double-weights one routing decision. Scope narrowing --------------- Cases carry the registration a change-scoped evaluation needs: Priority, Severity, Batch (derived from priority, with cross-cutting forced to batch 1), CrossCutting, InvolvedAgents, BusinessDomain, ExpectedOutcome and LastReviewedDate. Existing cases read back as P1/S1 -- mandatory but not stop-loss, which is the honest position for a case nobody has triaged. POST scope answers which cases a change needs to run. Every rule resolves towards including, because the two failure directions are not symmetrical: a case wrongly included costs tokens and is obvious, while one wrongly excluded produces no result at all, and "not run" is indistinguishable from "passed" once the numbers are in a report. Unknown involved agents therefore fail open, and both halves of the decision are returned with the rule that produced them. InvolvedAgents falls back to the case entry agent when unauthored, which is definitionally involved and already known -- so an Agent case is picked up by a change to its own agent without anyone maintaining a list. Latency, tokens and cost ------------------------ Each turn is timed around the agent call alone, and the case reports that separately from its wall clock, which also contains the canary and the conversation reads. The run summarises P50 and P95 per model at completion, nearest-rank so every figure is a duration some case actually took. Cases that never reached the model are excluded from the percentiles -- otherwise a run that mostly crashed reports the best latency on record -- but still counted in tokens and cost, which they really did spend. Token usage is read as a delta across the case rather than as an absolute, so a reused scope cannot bill one case for another tokens, and it is read in a finally block so a timed-out case still reports what it cost. Total only: the input/output split lives in TokenStatistics private fields and is not reachable through ITokenStatistics. The run also snapshots each model configured unit costs. A cost figure is not comparable with another run without them, and recording only a version string would leave nobody able to check whether two versions differ. Rate limiting ------------- BotSharp rate limiting counts human behaviour, and a regression suite does not look like one: it opens a conversation per case per model, drives turns as fast as the model answers, and runs in a BackgroundService whose user identity is empty -- which, because the Mongo filter drops an empty UserId rather than matching on it, measured the harness against every conversation in the instance. Every case failed with a message about conversation quotas that said nothing about the agent. ISyntheticConversationProbe (new, in Abstraction) lets a harness declare which conversations are its own, and RateLimitConversationHook stands aside for those on its two volume guards. The plugin answers from the run registry rather than from the conversation tag, because the tag is written only after the first message has already passed the hook. The input-length guard still applies: that one is about a single message being too large, which a test should surface rather than be excused from. A probe that throws fails closed, so a bug there cannot lift the limits for real traffic. Tests ----- 336 unit tests, up from 194. The ones worth reviewing are CaseScopeTests, which pins every narrowing rule in the direction of including, and SyntheticConversationExemptionTests, which checks that real traffic is still limited in every case where the harness is not. Co-Authored-By: Claude Opus 5 --- .../ISyntheticConversationProbe.cs | 31 + .../Hooks/RateLimitConversationHook.cs | 54 ++ .../AgentTestingPlugin.cs | 11 + .../Controllers/AgentTestController.cs | 310 ++++++- .../Models/AgentTestCase.cs | 232 +++++ .../Models/AgentTestCaseResult.cs | 72 ++ .../Models/AgentTestDtos.cs | 110 +++ .../Models/AgentTestRun.cs | 97 ++ .../AgentTestSyntheticConversationProbe.cs | 31 + .../Services/AgentTestCaseRunner.cs | 190 +++- .../Services/AgentTestRunExecutor.cs | 165 +++- .../Services/AssertionContext.cs | 38 +- .../Services/AssertionEvaluator.cs | 154 +++- .../BotSharpAgentConversationDriver.cs | 114 ++- .../Services/CaseScope.cs | 172 ++++ .../Services/IAgentConversationDriver.cs | 26 +- .../Services/IAgentTestJudge.cs | 55 ++ .../Services/LlmAgentTestJudge.cs | 236 +++++ .../AgentTesting/AgentChainAssertionTests.cs | 282 ++++++ .../AgentTesting/AgentTestCaseRunnerTests.cs | 500 +++++++++- .../AgentTesting/AgentTestControllerTests.cs | 859 +++++++++++++++++- .../AgentTesting/AgentTestJudgeTests.cs | 231 +++++ .../AgentTesting/AgentTestRunExecutorTests.cs | 279 +++++- .../AgentTesting/AssertionEvaluatorTests.cs | 15 +- .../AgentTesting/CaseScopeTests.cs | 245 +++++ .../SyntheticConversationExemptionTests.cs | 280 ++++++ .../BotSharp.Core.UnitTests.csproj | 2 + 27 files changed, 4744 insertions(+), 47 deletions(-) create mode 100644 src/Infrastructure/BotSharp.Abstraction/Conversations/ISyntheticConversationProbe.cs create mode 100644 src/Plugins/BotSharp.Plugin.AgentTesting/Runtime/AgentTestSyntheticConversationProbe.cs create mode 100644 src/Plugins/BotSharp.Plugin.AgentTesting/Services/CaseScope.cs create mode 100644 src/Plugins/BotSharp.Plugin.AgentTesting/Services/IAgentTestJudge.cs create mode 100644 src/Plugins/BotSharp.Plugin.AgentTesting/Services/LlmAgentTestJudge.cs create mode 100644 tests/BotSharp.Core.UnitTests/AgentTesting/AgentChainAssertionTests.cs create mode 100644 tests/BotSharp.Core.UnitTests/AgentTesting/AgentTestJudgeTests.cs create mode 100644 tests/BotSharp.Core.UnitTests/AgentTesting/CaseScopeTests.cs create mode 100644 tests/BotSharp.Core.UnitTests/AgentTesting/SyntheticConversationExemptionTests.cs diff --git a/src/Infrastructure/BotSharp.Abstraction/Conversations/ISyntheticConversationProbe.cs b/src/Infrastructure/BotSharp.Abstraction/Conversations/ISyntheticConversationProbe.cs new file mode 100644 index 000000000..d15ff6427 --- /dev/null +++ b/src/Infrastructure/BotSharp.Abstraction/Conversations/ISyntheticConversationProbe.cs @@ -0,0 +1,31 @@ +namespace BotSharp.Abstraction.Conversations; + +/// +/// Reports whether a conversation is synthetic -- driven by an automated harness rather than by a +/// person -- so that guards aimed at human overuse can stand aside for it. +/// +/// This exists because per-user volume limits count the wrong thing for a test harness. A regression +/// suite legitimately opens one conversation per case per model, at machine speed, and does it under +/// whatever identity the background worker happens to have. Measured as human behaviour that looks +/// like abuse, and the limit then fails every test with a message about conversation quotas -- which +/// says nothing about the agent under test and is indistinguishable, in a report, from the agent +/// having regressed. +/// +/// Deliberately a query rather than a flag on the message: a flag would have to be threaded through +/// every call that creates or continues a conversation, and anything that forgot would silently be +/// treated as human traffic. Asking by conversation id keeps the answer in one place. +/// +/// Implementations must be cheap and side-effect free -- this is called on the message path -- and +/// must answer false for anything they do not recognise. Several may be registered; a conversation is +/// synthetic if any of them claims it. None being registered is the normal case, and then nothing is +/// exempt. +/// +public interface ISyntheticConversationProbe +{ + /// + /// True when this conversation is being driven by a harness. Must not throw, and must not treat + /// an unknown or blank id as synthetic: getting this wrong in that direction exempts real user + /// traffic from the very limits it is meant to be held to. + /// + bool IsSynthetic(string conversationId); +} diff --git a/src/Infrastructure/BotSharp.Logger/Hooks/RateLimitConversationHook.cs b/src/Infrastructure/BotSharp.Logger/Hooks/RateLimitConversationHook.cs index 02cafbd08..5ea5fba0a 100644 --- a/src/Infrastructure/BotSharp.Logger/Hooks/RateLimitConversationHook.cs +++ b/src/Infrastructure/BotSharp.Logger/Hooks/RateLimitConversationHook.cs @@ -41,6 +41,19 @@ public override async Task OnMessageReceived(RoleDialogModel message) return; } + // Everything below this point is a volume guard against human overuse, and neither guard + // measures anything meaningful for an automated harness: a regression suite legitimately + // opens one conversation per case per model and drives its turns as fast as the model + // answers. Left in force they fail every test with a message about quotas, which says nothing + // about the agent under test and reads, in a report, exactly like the agent having regressed. + // + // The input length check above still applies. That one is about a single message being too + // large, which is a real condition a test should surface rather than be excused from. + if (IsSyntheticConversation()) + { + return; + } + // Check message sending frequency var userSents = Dialogs.Where(x => x.Role == AgentRole.User) .TakeLast(2).ToList(); @@ -75,4 +88,45 @@ public override async Task OnMessageReceived(RoleDialogModel message) } } } + + /// + /// Whether this conversation is driven by a harness rather than a person. False when nothing is + /// registered to answer, which is the normal case -- an absent probe must never exempt anyone. + /// + private bool IsSyntheticConversation() + { + var probes = _services.GetServices().ToList(); + if (probes.Count == 0) + { + return false; + } + + var conversationId = _services.GetRequiredService().ConversationId; + if (string.IsNullOrEmpty(conversationId)) + { + return false; + } + + foreach (var probe in probes) + { + try + { + if (probe.IsSynthetic(conversationId)) + { + return true; + } + } + catch (Exception ex) + { + // A probe that throws must not take the message down with it, and must not be read as + // a yes: failing closed here means the worst case is a harness message being rate + // limited, rather than real traffic escaping the limit. + _logger.LogWarning(ex, + "A synthetic conversation probe failed for conversation {ConversationId}; " + + "treating it as real traffic.", conversationId); + } + } + + return false; + } } diff --git a/src/Plugins/BotSharp.Plugin.AgentTesting/AgentTestingPlugin.cs b/src/Plugins/BotSharp.Plugin.AgentTesting/AgentTestingPlugin.cs index 865423073..1e3bdd742 100644 --- a/src/Plugins/BotSharp.Plugin.AgentTesting/AgentTestingPlugin.cs +++ b/src/Plugins/BotSharp.Plugin.AgentTesting/AgentTestingPlugin.cs @@ -44,6 +44,13 @@ public void RegisterDI(IServiceCollection services, IConfiguration config) // Singleton: the test context has to be visible across requests and across threads. services.AddSingleton(); + // Lets BotSharp's own rate limiting recognise a harness conversation and stand aside. Backed + // by the registry rather than by the conversation's "test-set" tag on purpose: the tag is + // only written once the conversation row exists, which is after the first message has already + // been through the rate limit hook, so a tag-based check would still block the first turn of + // every case. The registry entry exists before the conversation is opened at all. + services.AddSingleton(); + // The seam that takes over function execution. Lose this line and mocking silently stops // working -- the runner's canary self-check is what catches that. services.AddScoped(); @@ -76,6 +83,10 @@ public void RegisterDI(IServiceCollection services, IConfiguration config) // AgentTestController, so it needs an explicit registration the same way every other // service in this method does (there is no auto-discovery mechanism in this plugin). services.AddScoped(); + + // Scoped, like the segmenter: it resolves IChatCompletion implementations out of the same + // scope and holds no state between calls. + services.AddScoped(); services.AddScoped(); // AgentTestRunQueue is both a singleton and a BackgroundService: all three lines point at diff --git a/src/Plugins/BotSharp.Plugin.AgentTesting/Controllers/AgentTestController.cs b/src/Plugins/BotSharp.Plugin.AgentTesting/Controllers/AgentTestController.cs index 1d6315c17..54afeb01a 100644 --- a/src/Plugins/BotSharp.Plugin.AgentTesting/Controllers/AgentTestController.cs +++ b/src/Plugins/BotSharp.Plugin.AgentTesting/Controllers/AgentTestController.cs @@ -1,4 +1,6 @@ using System.Security.Claims; +using MongoDB.Bson; +using MongoDB.Bson.Serialization; using BotSharp.Abstraction.Agents; using BotSharp.Abstraction.MLTasks; using BotSharp.Abstraction.Infrastructures.Attributes; @@ -127,6 +129,11 @@ public async Task> CreateCase([FromBody] AgentTestCa return BadRequest("suiteId is required"); } + if (await ValidateEntryAgentAsync(request) is { } entryAgentError) + { + return BadRequest(entryAgentError); + } + if (ValidateCasePayload(request) is { } validationError) { return BadRequest(validationError); @@ -166,6 +173,11 @@ public async Task> UpdateCase(string id, [FromBody] return NotFound($"agent test case {id} not found"); } + if (await ValidateEntryAgentAsync(request) is { } entryAgentError) + { + return BadRequest(entryAgentError); + } + if (ValidateCasePayload(request) is { } validationError) { return BadRequest(validationError); @@ -193,6 +205,157 @@ public async Task> UpdateCase(string id, [FromBody] return testCase; } + /// + /// Duplicates a case inside its own suite and returns the copy. + /// + /// Server-side rather than a client GET-then-POST because the copy has to carry EVERY field the + /// case has. A client that rebuilds the payload from its own form drops whatever it does not know + /// about, and a copy missing its mocks is indistinguishable in the list from a correct one -- + /// right up to the run where it blocks every tool the agent reaches for. + /// + /// Not cross-suite on purpose: moving a case between suites also changes which agent it runs + /// against, so the copy would need a different entry agent and different mock targets to mean + /// anything. That is an edit, not a copy. + /// + /// + /// Which cases a change needs to run, and -- just as importantly -- which it does not, with the + /// reason for each. + /// + /// Read-only and side-effect free: it plans a run, it does not start one. Triggering stays + /// per-suite (POST suites/{id}/run), so a caller takes the included case ids from here and + /// triggers each suite that appears among them. + /// + [HttpPost("scope")] + public async Task> SelectScope([FromBody] ScopeSelectionRequest request) + { + request ??= new ScopeSelectionRequest(); + + if (request.Batch is { } batch && !CaseBatches.All.Contains(batch)) + { + return BadRequest($"batch must be one of {string.Join(", ", CaseBatches.All)}, not {batch}"); + } + + var targets = (request.TargetAgentIds ?? []) + .Where(id => !string.IsNullOrWhiteSpace(id)) + .Select(id => id.Trim()) + .Distinct(StringComparer.OrdinalIgnoreCase) + .ToList(); + + // Naming no agents and not declaring a platform-wide change would narrow to nothing at all, + // and an empty scope reported as a successful plan is the single most dangerous answer this + // endpoint could give. + if (targets.Count == 0 && !request.FullPlatform) + { + return BadRequest( + "name at least one target agent, or set fullPlatform for a change with no single target"); + } + + var query = new ScopeQuery + { + TargetAgentIds = targets, + FullPlatform = request.FullPlatform, + Batch = request.Batch + }; + + var response = new ScopeSelectionResponse + { + TargetAgentIds = targets, + FullPlatform = request.FullPlatform, + Batch = request.Batch + }; + + foreach (var suite in await _repo.ListSuitesAsync(null)) + { + foreach (var testCase in await _repo.ListCasesAsync(suite.Id)) + { + var decision = CaseScope.Decide(testCase, suite.AgentId, query); + var dto = new ScopedCaseDto + { + CaseId = testCase.Id, + CaseName = testCase.Name, + SuiteId = suite.Id, + SuiteName = suite.Name, + CaseType = testCase.CaseType, + Priority = testCase.Priority, + Severity = testCase.Severity, + CrossCutting = testCase.CrossCutting, + Enabled = testCase.Enabled, + Batch = decision.Batch, + InvolvedAgentIds = decision.InvolvedAgentIds.ToList(), + Reason = decision.Reason + }; + + response.TotalCases++; + (decision.Included ? response.Included : response.Excluded).Add(dto); + } + } + + return response; + } + + [HttpPost("cases/{id}/copy")] + public async Task> CopyCase(string id) + { + var source = await _repo.GetCaseAsync(id); + if (source == null) + { + return NotFound($"agent test case {id} not found"); + } + + // Cloned by a BSON round trip rather than field by field. A hand-written clone has exactly + // the drift problem this endpoint exists to prevent: the next field added to AgentTestCase + // would be silently absent from every copy, and nothing would fail until a run. + var copy = BsonSerializer.Deserialize(source.ToBsonDocument()); + + // Blank so UpsertCaseAsync mints a new one -- the round trip copied the source's _id, and + // keeping it would make the "copy" a full overwrite of the original. + copy.Id = string.Empty; + + var siblings = await _repo.ListCasesAsync(source.SuiteId); + copy.Name = NextCopyName(source.Name, siblings.Select(c => c.Name)); + + // Disabled regardless of the source. An exact duplicate that joins the next run measures the + // same thing twice: it pads the pass-rate denominator, and for a routing case it + // double-weights one routing decision. A copy is made in order to be edited into a variant, + // so it waits for that edit -- the same reason a recorded draft lands disabled. + copy.Enabled = false; + + // The round trip also copied CreateDate, which would have the copy claim to be as old as the + // case it came from. UpdateDate is set by the repository on write. + copy.CreateDate = DateTime.UtcNow; + + await _repo.UpsertCaseAsync(copy); + return copy; + } + + /// + /// "x" becomes "x (copy)", then "x (copy 2)", "x (copy 3)". Names are not unique in this store, + /// so this is presentation rather than a constraint -- but two rows both called "x (copy)" are + /// impossible to tell apart in the list, which is the one place copies are managed. + /// + /// Capped at the length the case editor's own input accepts, so the copy stays editable: a name + /// the form cannot hold would have to be trimmed by hand before any other change could be saved. + /// + private static string NextCopyName(string name, IEnumerable existing) + { + const int maxNameLength = 200; + + var taken = new HashSet(existing, StringComparer.OrdinalIgnoreCase); + + for (var attempt = 1; ; attempt++) + { + var suffix = attempt == 1 ? " (copy)" : $" (copy {attempt})"; + var room = maxNameLength - suffix.Length; + var stem = name.Length > room ? name[..room] : name; + var candidate = stem + suffix; + + if (!taken.Contains(candidate)) + { + return candidate; + } + } + } + [HttpDelete("cases/{id}")] public async Task DeleteCase(string id) { @@ -443,12 +606,47 @@ private static void ApplyCase(AgentTestCase testCase, AgentTestCaseUpsertRequest testCase.SuiteId = request.SuiteId; testCase.Name = request.Name; testCase.Enabled = request.Enabled; + // Normalised, not taken verbatim: ValidateCasePayload has already rejected anything that is + // neither blank nor a known type, so this only fixes casing ("routing" -> "Routing") and + // maps blank onto the default. Storing the caller's casing would break every later + // Ordinal comparison against CaseTypes.Routing. + testCase.CaseType = CaseTypes.Normalize(request.CaseType) ?? CaseTypes.Agent; + testCase.EntryAgentId = string.IsNullOrWhiteSpace(request.EntryAgentId) ? null : request.EntryAgentId.Trim(); testCase.Turns = request.Turns ?? []; testCase.Assertions = request.Assertions ?? []; testCase.InitialStates = request.InitialStates ?? []; + testCase.History = (request.History ?? []) + .Select(m => new TestHistoryMessage + { + // Normalised for the same reason CaseType is: the driver and every later comparison + // use the canonical lowercase constants. + Role = HistoryRoles.Normalize(m.Role) ?? HistoryRoles.User, + Content = m.Content ?? string.Empty + }) + .ToList(); testCase.Mocks = request.Mocks ?? []; testCase.UnmockedToolPolicy = request.UnmockedToolPolicy; testCase.SourceConversationId = request.SourceConversationId; + + // Normalised, not verbatim: validation has already rejected anything unrecognised, so this + // only fixes casing. Storing "p0" would leave a case that never matches an Ordinal comparison + // against CasePriorities.P0 -- it would run, and then be filed in the wrong batch. + testCase.Priority = CasePriorities.Normalize(request.Priority) ?? CasePriorities.P1; + testCase.Severity = CaseSeverities.Normalize(request.Severity) ?? CaseSeverities.S1; + testCase.Batch = request.Batch; + testCase.CrossCutting = request.CrossCutting; + testCase.InvolvedAgents = (request.InvolvedAgents ?? []) + .Where(id => !string.IsNullOrWhiteSpace(id)) + .Select(id => id.Trim()) + .Distinct(StringComparer.OrdinalIgnoreCase) + .ToList(); + testCase.BusinessDomain = string.IsNullOrWhiteSpace(request.BusinessDomain) + ? null + : request.BusinessDomain.Trim(); + testCase.ExpectedOutcome = string.IsNullOrWhiteSpace(request.ExpectedOutcome) + ? null + : request.ExpectedOutcome.Trim(); + testCase.LastReviewedDate = request.LastReviewedDate; } /// @@ -462,9 +660,51 @@ private static void ApplyCase(AgentTestCase testCase, AgentTestCaseUpsertRequest return "Passthrough is not supported in P1"; } + var caseType = CaseTypes.Normalize(request.CaseType); + if (caseType == null && !string.IsNullOrWhiteSpace(request.CaseType)) + { + return $"caseType must be one of {string.Join(", ", CaseTypes.All)}, not '{request.CaseType}'"; + } + + if (CasePriorities.Normalize(request.Priority) == null && !string.IsNullOrWhiteSpace(request.Priority)) + { + return $"priority must be one of {string.Join(", ", CasePriorities.All)}, not '{request.Priority}'"; + } + + if (CaseSeverities.Normalize(request.Severity) == null && !string.IsNullOrWhiteSpace(request.Severity)) + { + return $"severity must be one of {string.Join(", ", CaseSeverities.All)}, not '{request.Severity}'"; + } + + // Rejected rather than clamped: a batch of 4 is a mistake, and silently filing the case in + // batch 3 would leave the author believing it runs somewhere it does not. + if (request.Batch is { } batch && !CaseBatches.All.Contains(batch)) + { + return $"batch must be one of {string.Join(", ", CaseBatches.All)}, not {batch}"; + } + + foreach (var (message, index) in (request.History ?? []).Select((m, i) => (m, i))) + { + if (HistoryRoles.Normalize(message?.Role) == null) + { + return $"history message {index + 1} has role '{message?.Role}'; only " + + $"{string.Join(" and ", HistoryRoles.All)} are supported"; + } + + // An empty message is dropped by BotSharp's own dialog storage (ConversationStorage + // skips elements with blank content), so it would silently not be there at run time -- + // and the runner's count check would then fail the whole case with a confusing message + // about the write having vanished. + if (string.IsNullOrWhiteSpace(message!.Content)) + { + return $"history message {index + 1} has no content"; + } + } + var allAssertions = (request.Turns ?? []) .SelectMany(t => t.Assertions ?? []) - .Concat(request.Assertions ?? []); + .Concat(request.Assertions ?? []) + .ToList(); foreach (var assertion in allAssertions) { @@ -475,9 +715,77 @@ private static void ApplyCase(AgentTestCase testCase, AgentTestCaseUpsertRequest } } + return ValidateRoutingCase(caseType ?? CaseTypes.Agent, request, allAssertions); + } + + /// + /// The extra rules a Routing case has to satisfy. A Routing case is not a label -- it is the only + /// type counted towards a run's routing accuracy, so one that cannot actually establish a routing + /// outcome would quietly move that figure without measuring anything. + /// + /// Enforced at save time for the same reason AssertionValidation is: the alternative is a case + /// that saves cleanly and then reports a meaningless green every run. + /// + private static string? ValidateRoutingCase( + string caseType, AgentTestCaseUpsertRequest request, List allAssertions) + { + if (!string.Equals(caseType, CaseTypes.Routing, StringComparison.Ordinal)) + { + return null; + } + + // Routing is a single-turn question: which agent picks this message up. A second turn is + // either a different question (making it an Agent case) or an accident, and either way its + // result would be counted as routing accuracy. + // + // Authored History is not a turn and is deliberately not counted here: replaying a prior + // exchange and then asking one question is still a single routing decision, and it is the + // most realistic way to test routing that depends on context. + if ((request.Turns ?? []).Count != 1) + { + return "a Routing case must have exactly one turn; use an Agent case for a multi-turn case"; + } + + // Without one of these the case asserts nothing about routing, yet still counts towards + // routing accuracy -- it would report Passed for having successfully said anything at all. + var assertsRouting = allAssertions.Any(a => + string.Equals(a.Type, AssertionTypes.RoutedToAgent, StringComparison.Ordinal) + || string.Equals(a.Type, AssertionTypes.AgentChain, StringComparison.Ordinal)); + if (!assertsRouting) + { + return $"a Routing case needs at least one '{AssertionTypes.RoutedToAgent}' or " + + $"'{AssertionTypes.AgentChain}' assertion, otherwise it verifies no routing outcome"; + } + + // The framework this implements scores routing purely as expected-agent == actual-agent and + // deliberately applies no quality judgement to it. An llmJudge here would also make the + // routing figure depend on a vendor call, so a vendor outage would read as a routing + // regression. + if (allAssertions.Any(a => string.Equals(a.Type, AssertionTypes.LlmJudge, StringComparison.Ordinal))) + { + return $"a Routing case cannot use '{AssertionTypes.LlmJudge}': routing is judged only by " + + "which agent handled the conversation"; + } + return null; } + /// + /// Null when the request's entry agent is usable. Separate from + /// because it needs IAgentService, so it cannot be static, and + /// a lookup is worth doing only once the cheap checks have passed. + /// + private async Task ValidateEntryAgentAsync(AgentTestCaseUpsertRequest request) + { + if (string.IsNullOrWhiteSpace(request.EntryAgentId)) + { + return null; + } + + var agent = await _agents.GetAgent(request.EntryAgentId.Trim()); + return agent == null ? $"entry agent {request.EntryAgentId} not found" : null; + } + /// /// Passthrough was specified in the design/plan and even had a (dead) code path, but nothing /// ever back-fills an ObservedToolCall for a tool the provider let run for real -- under it, diff --git a/src/Plugins/BotSharp.Plugin.AgentTesting/Models/AgentTestCase.cs b/src/Plugins/BotSharp.Plugin.AgentTesting/Models/AgentTestCase.cs index 1bc7db5cb..11394540d 100644 --- a/src/Plugins/BotSharp.Plugin.AgentTesting/Models/AgentTestCase.cs +++ b/src/Plugins/BotSharp.Plugin.AgentTesting/Models/AgentTestCase.cs @@ -8,6 +8,30 @@ public class AgentTestCase : MongoBase public string Name { get; set; } = default!; public bool Enabled { get; set; } = true; + /// + /// See . Defaults to Agent, which is also what every case stored before + /// this field existed deserialises to -- a missing BSON element leaves the property at this + /// initialiser, so old documents read back as Agent cases rather than as an invalid blank. + /// + /// Not cosmetic: a Routing case is validated differently (single turn, must actually assert a + /// routing outcome, no llmJudge -- see AgentTestController.ValidateCasePayload) and is the only + /// type counted towards a run's routing accuracy. + /// + public string CaseType { get; set; } = CaseTypes.Agent; + + /// + /// The agent the conversation is opened on, overriding the suite's own AgentId. Null uses the + /// suite's. + /// + /// This is what makes Routing and Agent cases separable without one suite per entry point. + /// BotSharp dispatches on the agent's own type: ConversationService.SendMessage sends a Routing + /// agent through RoutingService.InstructLoop (the router runs, and can hand off) and everything + /// else through InstructDirect (straight into that agent, router never consulted). So pointing a + /// case at the Copilot entry agent tests routing, and pointing it at a leaf agent tests that + /// agent in isolation -- the two modes the evaluation framework calls Routing and Agent cases. + /// + public string? EntryAgentId { get; set; } + /// A length of 1 is a single-turn case. public List Turns { get; set; } = []; @@ -17,6 +41,22 @@ public class AgentTestCase : MongoBase /// Injected before the conversation starts; maps to BotSharp's MessageState. public List InitialStates { get; set; } = []; + /// + /// Prior conversation turns written into the conversation before the case's own turns run, so a + /// real question-and-answer exchange can be replayed as the fixed starting context for a test. + /// + /// Different from : states are the machine-readable context an agent + /// reads, this is the dialogue the model sees. It is what makes "given the resident already told + /// us the fridge is leaking, does asking for an ETA still route correctly" expressible without + /// re-driving those turns through the model on every run -- which would be slow, would cost + /// tokens, and would make the preamble itself a source of flakiness. + /// + /// Never counted in a result's AgentChain: these messages are authored, not something the agent + /// under test did, and letting them in would fail an exact chain assertion for a reason the + /// author did not cause. + /// + public List History { get; set; } = []; + public List Mocks { get; set; } = []; /// @@ -28,6 +68,68 @@ public class AgentTestCase : MongoBase /// The conversation this was recorded from, for traceability; null when hand-written. public string? SourceConversationId { get; set; } + /// + /// See . Decides which batch the case runs in, and therefore whether + /// a failure stops the evaluation (batch 1) or is merely reported (batch 3). + /// + /// Defaults to P1, which is what every case stored before this field existed reads back as. P0 + /// would put all of them in the stop-loss batch, where one failure halts everything; P2 would + /// drop them out of the mandatory batches altogether. P1 is mandatory but not stop-loss, which is + /// the honest position for a case nobody has triaged yet. + /// + public string Priority { get; set; } = CasePriorities.P1; + + /// + /// See . What a failure of this case means, as opposed to how urgent + /// it is to run. + /// + /// Defaults to S1 for the same reason as Priority: S0 would make every untriaged legacy failure + /// an immediate no-go, and S2 would let a genuine safety failure hide as an experience nit. + /// + public string Severity { get; set; } = CaseSeverities.S1; + + /// + /// Overrides the batch derived from and ; null + /// uses the derivation. See . + /// + public int? Batch { get; set; } + + /// + /// A cross-cutting case runs in EVERY evaluation scope, whatever the change was. That is what + /// safety cases are: the scope-narrowing rules exist to save time on cases a change cannot + /// affect, and a claim that a change cannot affect safety is exactly the claim not to take on + /// trust. + /// + public bool CrossCutting { get; set; } + + /// + /// Agents this case actually exercises, as ids. Empty is the normal state and does NOT mean "no + /// agents": the effective set then falls back to the case's entry agent, which is already known. + /// See . + /// + /// Worth filling in for a routing case, where the entry agent is the router and the agents that + /// matter are the ones downstream of it -- those cannot be inferred from the case definition, + /// only observed by running it. + /// + public List InvolvedAgents { get; set; } = []; + + /// Business domain, for pulling a subset by business line rather than by agent. + public string? BusinessDomain { get; set; } + + /// + /// What this case is supposed to achieve, in business terms, for whoever reviews the result. + /// Deliberately free text and never evaluated: an expected outcome a machine could check is an + /// assertion, and belongs in where it will actually be enforced. + /// + public string? ExpectedOutcome { get; set; } + + /// + /// When a human last confirmed this case still reflects reality. Null means never. Not touched by + /// editing or running the case -- a case can be edited many times and still be built on an + /// assumption nobody has questioned in a year, and conflating the two would hide exactly that. + /// + public DateTime? LastReviewedDate { get; set; } + public DateTime CreateDate { get; set; } = DateTime.UtcNow; public DateTime UpdateDate { get; set; } = DateTime.UtcNow; } @@ -100,11 +202,141 @@ public class TestAssertion public bool Fatal { get; set; } } +/// +/// One authored message in a case's . Deliberately just a role +/// and text: a mocked tool call belongs in , and a fabricated +/// function-call dialog would let a case claim a tool ran when nothing did. +/// +[BsonIgnoreExtraElements(Inherited = true)] +public class TestHistoryMessage +{ + /// See . + public string Role { get; set; } = HistoryRoles.User; + + public string Content { get; set; } = string.Empty; +} + +/// +/// Roles an authored history message may take. Only these two: a system message would compete with +/// the agent's own instruction, and a function message would fake a tool call. +/// +public static class HistoryRoles +{ + public const string User = "user"; + public const string Assistant = "assistant"; + + public static readonly string[] All = [User, Assistant]; + + /// Canonical role for any casing; null for anything unsupported, so it is rejected. + public static string? Normalize(string? value) + => All.FirstOrDefault(r => string.Equals(r, value?.Trim(), StringComparison.OrdinalIgnoreCase)); +} + public static class UnmockedToolPolicies { public const string Block = "Block"; } +/// +/// How urgent it is to run a case, which is what decides its batch. Distinct from +/// : priority is about scheduling, severity is about consequence. +/// +public static class CasePriorities +{ + public const string P0 = "P0"; + public const string P1 = "P1"; + public const string P2 = "P2"; + + public static readonly string[] All = [P0, P1, P2]; + + public static string? Normalize(string? value) + => All.FirstOrDefault(p => string.Equals(p, value?.Trim(), StringComparison.OrdinalIgnoreCase)); +} + +/// +/// What a failure of this case means. +/// +/// S0 -- zero tolerance. Data leakage, an unauthorised action taken without confirmation, a missed +/// critical escalation. One of these is a stop, not a statistic. +/// S1 -- non-inferiority. Wrong routing, wrong tool, a fabricated fact, a wrong business state: +/// allowed to move within a threshold, not allowed to get worse than it. +/// S2 -- experience quality. Phrasing, repetition, awkward hand-offs. Must never be able to mask an +/// S0 or S1 result. +/// +public static class CaseSeverities +{ + public const string S0 = "S0"; + public const string S1 = "S1"; + public const string S2 = "S2"; + + public static readonly string[] All = [S0, S1, S2]; + + public static string? Normalize(string? value) + => All.FirstOrDefault(s => string.Equals(s, value?.Trim(), StringComparison.OrdinalIgnoreCase)); +} + +/// +/// Which batch a case belongs to. Batches run in order and exist to stop early: batch 1 is the +/// stop-loss batch, batch 3 does not block a release decision. +/// +public static class CaseBatches +{ + public const int StopLoss = 1; + public const int Mandatory = 2; + public const int Optional = 3; + + public static readonly int[] All = [StopLoss, Mandatory, Optional]; + + /// + /// An explicit wins. Otherwise a cross-cutting case is batch 1 + /// whatever its priority -- a safety case that only runs after everything else has passed cannot + /// stop anything -- and priority maps P0/P1/P2 onto 1/2/3. + /// + public static int Effective(AgentTestCase testCase) + { + if (testCase.Batch is { } explicitBatch && All.Contains(explicitBatch)) + { + return explicitBatch; + } + + if (testCase.CrossCutting) + { + return StopLoss; + } + + return testCase.Priority switch + { + CasePriorities.P0 => StopLoss, + CasePriorities.P2 => Optional, + _ => Mandatory + }; + } +} + +/// +/// What a case is verifying, which decides how it is validated and how it is aggregated. +/// +/// Routing -- single turn from the entry agent, asserting only which agent took the conversation. +/// Carries no quality judgement: the whole verdict is "expected agent == actual agent". +/// Agent -- one agent's own behaviour, normally entered directly so the router is not part of what +/// is being measured. Multi-agent journeys are Agent cases too; the agentChain assertion +/// is what describes their hand-offs. +/// +public static class CaseTypes +{ + public const string Routing = "Routing"; + public const string Agent = "Agent"; + + public static readonly string[] All = [Routing, Agent]; + + /// + /// Maps any casing of a known type onto its canonical constant; null for anything unknown, so + /// the caller can reject it rather than storing a value nothing else will ever match. + /// + public static string? Normalize(string? value) + => All.FirstOrDefault(t => string.Equals(t, value?.Trim(), StringComparison.OrdinalIgnoreCase)); +} + public static class AgentTestStatus { public const string Pending = "Pending"; diff --git a/src/Plugins/BotSharp.Plugin.AgentTesting/Models/AgentTestCaseResult.cs b/src/Plugins/BotSharp.Plugin.AgentTesting/Models/AgentTestCaseResult.cs index bea9793ed..2b04b3684 100644 --- a/src/Plugins/BotSharp.Plugin.AgentTesting/Models/AgentTestCaseResult.cs +++ b/src/Plugins/BotSharp.Plugin.AgentTesting/Models/AgentTestCaseResult.cs @@ -11,6 +11,14 @@ public class AgentTestCaseResult : MongoBase /// Passed | Failed | Error | Cancelled -- see . public string Status { get; set; } = AgentTestStatus.Pending; + /// + /// Copied off the case so a result is self-describing -- see . Aggregating + /// by type (routing accuracy separately from agent pass rate, as the evaluation framework gates + /// them separately) has to work from the result rows alone, without re-reading cases that may + /// have been edited or deleted since the run. + /// + public string CaseType { get; set; } = CaseTypes.Agent; + /// The conversation this execution created; live conversations are never reused. public string? ConversationId { get; set; } @@ -23,8 +31,37 @@ public class AgentTestCaseResult : MongoBase public string? Provider { get; set; } public string? Model { get; set; } + /// + /// Wall clock for the whole case, including the canary, the mock lookups and the conversation + /// reads. Comparable between models because every model pays the same overhead, but it is not a + /// model-latency measurement -- see for that. + /// public long DurationMs { get; set; } + /// + /// Time spent inside the agent calls alone, summed over the turns. This is what a latency gate + /// should be read against: also contains harness work, and on a fast + /// case that overhead is a large enough share to move a percentile. + /// + public long ModelDurationMs { get; set; } + + /// + /// Tokens this case consumed, measured as the delta across its own execution rather than an + /// absolute reading, so it stays correct even when the statistics service outlives one case. + /// + /// Total only: the input/output split lives in TokenStatistics' private fields and is not + /// reachable through ITokenStatistics, which exposes Total, Cost and AccumulatedCost. Recording a + /// guessed split would be worse than recording none. + /// + public long TotalTokens { get; set; } + + /// + /// What those tokens cost, priced by the model's own configured unit costs. Comparable only + /// within a run, or across runs whose pricing snapshot matches -- see + /// . + /// + public double Cost { get; set; } + /// /// Infrastructure-level reason for failure (a timeout, a dead canary), kept distinct from an /// assertion failure. @@ -38,6 +75,19 @@ public class AgentTestCaseResult : MongoBase public List ObservedToolCalls { get; set; } = []; + /// + /// Every agent that produced an assistant message over the whole case, in order, with + /// consecutive repeats collapsed -- so ["Copilot", "WorkOrder"] means the entry agent answered + /// and then handed off once, however many messages each of them emitted. + /// + /// This is the only record of the hand-offs. route_to_agent is on the allow list + /// (AgentTestRunRegistry) and therefore never reaches MockFunctionExecutor, which is the only + /// caller of ActiveTestRun.Record -- so routing decisions produce no ObservedToolCall and would + /// otherwise be invisible. Reconstructed instead from the conversation's own assistant dialogs, + /// each of which carries the agent that wrote it. + /// + public List AgentChain { get; set; } = []; + public DateTime CreateDate { get; set; } = DateTime.UtcNow; } @@ -48,6 +98,21 @@ public class TurnResult public string UserMessage { get; set; } = default!; public string? Output { get; set; } public List Assertions { get; set; } = []; + + /// + /// Time the agent call for this turn took. Excludes the assertion evaluation and the conversation + /// reads that follow it, so summing this over the turns gives a latency figure that does not + /// drift as the harness itself gains work. + /// + public long ModelDurationMs { get; set; } + + /// + /// The agents that answered during THIS turn only, in order, consecutive repeats collapsed. The + /// case-level is the whole conversation; this is + /// the slice added by this turn, which is what makes "the second turn should have stayed with + /// the same agent" expressible. + /// + public List AgentChain { get; set; } = []; } [BsonIgnoreExtraElements(Inherited = true)] @@ -59,6 +124,13 @@ public class AssertionResult public string? Actual { get; set; } public bool Passed { get; set; } public string? Message { get; set; } + + /// + /// The numeric score, on llmJudge results only; null everywhere else. Actual already carries it + /// as text for display, but a quality gate has to average scores across a run, and re-parsing a + /// display string to do arithmetic is how a formatting change quietly breaks a gate. + /// + public double? Score { get; set; } } [BsonIgnoreExtraElements(Inherited = true)] diff --git a/src/Plugins/BotSharp.Plugin.AgentTesting/Models/AgentTestDtos.cs b/src/Plugins/BotSharp.Plugin.AgentTesting/Models/AgentTestDtos.cs index e8b6c2b9a..0ca4f1ad3 100644 --- a/src/Plugins/BotSharp.Plugin.AgentTesting/Models/AgentTestDtos.cs +++ b/src/Plugins/BotSharp.Plugin.AgentTesting/Models/AgentTestDtos.cs @@ -37,12 +37,122 @@ public class AgentTestCaseUpsertRequest public string SuiteId { get; set; } = string.Empty; public string Name { get; set; } = string.Empty; public bool Enabled { get; set; } = true; + + /// + /// See . Blank or omitted means Agent, so an existing client that never + /// sends this field keeps creating exactly the cases it created before. Any other unrecognised + /// value is a 400 rather than a silent fallback -- storing "Rounting" as Agent would leave the + /// author with a case that looks routing-shaped and is never counted as one. + /// + public string? CaseType { get; set; } + + /// + /// Authored prior turns, replayed as the case's opening context. See + /// . + /// + public List History { get; set; } = []; + + /// + /// Optional entry agent, overriding the suite's. See + /// for why this is the switch between testing routing + /// and testing one agent in isolation. Validated to exist at save time: a typo here would + /// otherwise turn every run of the case into an opaque infrastructure Error. + /// + public string? EntryAgentId { get; set; } + public List Turns { get; set; } = []; public List Assertions { get; set; } = []; public List InitialStates { get; set; } = []; public List Mocks { get; set; } = []; public string UnmockedToolPolicy { get; set; } = UnmockedToolPolicies.Block; public string? SourceConversationId { get; set; } + + /// See . Blank or omitted keeps P1. + public string? Priority { get; set; } + + /// See . Blank or omitted keeps S1. + public string? Severity { get; set; } + + /// Explicit batch override; null derives it from priority and the cross-cutting flag. + public int? Batch { get; set; } + + public bool CrossCutting { get; set; } + + /// Agent ids; empty falls back to the case's entry agent. See . + public List InvolvedAgents { get; set; } = []; + + public string? BusinessDomain { get; set; } + public string? ExpectedOutcome { get; set; } + + /// + /// Sent explicitly by whoever reviewed the case. Never set by the server on save: a case can be + /// edited many times and still rest on an assumption nobody has questioned, and stamping this on + /// every write would hide precisely that. + /// + public DateTime? LastReviewedDate { get; set; } +} + +/// +/// Body of POST /agent-test/scope -- work out which cases a change needs to run, before running +/// anything. See . +/// +public class ScopeSelectionRequest +{ + /// Agent ids the change touches. Ignored when is set. + public List TargetAgentIds { get; set; } = []; + + /// A platform-wide change: every enabled case is in scope. + public bool FullPlatform { get; set; } + + /// Narrow to one batch (1, 2 or 3); null covers all of them. + public int? Batch { get; set; } +} + +/// +/// One case's place in a scope. Carries the metadata the decision was made from, not just the verdict, +/// so the scope can be reviewed rather than taken on trust. +/// +public class ScopedCaseDto +{ + public string CaseId { get; set; } = string.Empty; + public string CaseName { get; set; } = string.Empty; + public string SuiteId { get; set; } = string.Empty; + public string SuiteName { get; set; } = string.Empty; + public string CaseType { get; set; } = string.Empty; + public string Priority { get; set; } = string.Empty; + public string Severity { get; set; } = string.Empty; + public bool CrossCutting { get; set; } + public bool Enabled { get; set; } + + /// The effective batch, after the priority and cross-cutting derivation. + public int Batch { get; set; } + + /// The set the decision was made against, authored or derived from the entry agent. + public List InvolvedAgentIds { get; set; } = []; + + /// See . + public string Reason { get; set; } = string.Empty; +} + +/// +/// The answer to "what will this change actually test". +/// +/// Both lists are returned on purpose. A scope report that only showed what it included would let a +/// change be signed off against a set of cases that quietly left out the interesting one, and an +/// excluded case produces no result to notice -- which is why the exclusions, with their reasons, are +/// the half worth reading. +/// +public class ScopeSelectionResponse +{ + public List TargetAgentIds { get; set; } = []; + public bool FullPlatform { get; set; } + public int? Batch { get; set; } + + /// Every registered case, whatever its state -- the denominator for coverage. + public int TotalCases { get; set; } + + public List Included { get; set; } = []; + public List Excluded { get; set; } = []; } /// diff --git a/src/Plugins/BotSharp.Plugin.AgentTesting/Models/AgentTestRun.cs b/src/Plugins/BotSharp.Plugin.AgentTesting/Models/AgentTestRun.cs index 31b3f6af5..2111649d4 100644 --- a/src/Plugins/BotSharp.Plugin.AgentTesting/Models/AgentTestRun.cs +++ b/src/Plugins/BotSharp.Plugin.AgentTesting/Models/AgentTestRun.cs @@ -46,6 +46,37 @@ public class AgentTestRun : MongoBase public int FailedCount { get; set; } public int ErrorCount { get; set; } + /// + /// Routing accuracy for this run, one row per model swept, counting only cases whose CaseType is + /// Routing. + /// + /// Kept per model rather than as one run-wide number because that is the whole point of a + /// comparison run: a single figure covering every model averages the candidate together with the + /// baseline and hides exactly the difference the run exists to measure. + /// + /// A summary of the case results, not a second source of truth -- the AgentTestCaseResult rows + /// remain authoritative and carry CaseType themselves. It lives here so the run LIST can show + /// the figure without loading every result of every run. + /// + public List RoutingAccuracies { get; set; } = []; + + /// + /// Latency, token and cost figures for this run, one row per model swept. Computed once when the + /// run finishes rather than accumulated per case, because a percentile cannot be updated + /// incrementally -- it needs every value at once. + /// + public List PerformanceSummaries { get; set; } = []; + + /// + /// The unit costs actually in force for each model when this run executed. + /// + /// A cost figure is meaningless without them: a provider price change makes this run's cost + /// incomparable with an earlier one's, and a run that only recorded a version STRING would leave + /// nobody able to check whether two versions differ. Snapshotting the numbers makes that + /// checkable instead of a matter of trust. + /// + public List ModelPricing { get; set; } = []; + /// /// Why a run ended as -- an infrastructure stop that /// happened before or instead of executing cases (suite gone, suite disabled, the CaseIds @@ -63,3 +94,69 @@ public class AgentTestRun : MongoBase public DateTime? CompletedAt { get; set; } public DateTime CreateDate { get; set; } = DateTime.UtcNow; } + +/// +/// Latency, tokens and cost for one model within a run. Sums and percentiles are stored; averages are +/// not, because an average is Total/CaseCount and a stored copy is one more thing that can disagree +/// with the rows it came from. +/// +[BsonIgnoreExtraElements(Inherited = true)] +public class PerformanceSummary +{ + /// Null for both when the run swept no models and used each agent's own LlmConfig. + public string? Provider { get; set; } + public string? Model { get; set; } + + /// + /// Results this row covers. Only cases that actually executed: an Error case that never reached + /// the model would drag a latency percentile towards zero and make a broken run look fast. + /// + public int CaseCount { get; set; } + + /// Median and 95th percentile of AgentTestCaseResult.ModelDurationMs. + public long LatencyP50Ms { get; set; } + public long LatencyP95Ms { get; set; } + + public long TotalTokens { get; set; } + public double TotalCost { get; set; } +} + +/// +/// One model's configured unit costs at the moment a run executed. Text tokens only -- the harness +/// drives text conversations, and carrying audio and image tiers that are always zero here would +/// suggest they had been checked. +/// +[BsonIgnoreExtraElements(Inherited = true)] +public class ModelPricingSnapshot +{ + public string? Provider { get; set; } + public string? Model { get; set; } + + /// Null when the model's settings could not be read, which is itself worth recording. + public float? TextInputCost { get; set; } + public float? TextOutputCost { get; set; } +} + +/// +/// How many Routing cases one model got right in a run. Stored as counts, never as a percentage: +/// a stored ratio would go stale the moment another case result arrives, and "3/4" says something +/// "75%" does not -- how much the figure is worth trusting. +/// +[BsonIgnoreExtraElements(Inherited = true)] +public class RoutingAccuracy +{ + /// Null for both when the run swept no models and used each agent's own LlmConfig. + public string? Provider { get; set; } + public string? Model { get; set; } + + /// Routing cases executed under this model, whatever their outcome. + public int CaseCount { get; set; } + + /// + /// Of those, how many passed. A routing case passes only when its routing assertions held, so + /// Passed here means the conversation reached the expected agent. Error rows (a timeout, a dead + /// canary) count towards CaseCount but never towards PassedCount -- treating "could not tell" + /// as correct is how a broken harness starts reporting perfect accuracy. + /// + public int PassedCount { get; set; } +} diff --git a/src/Plugins/BotSharp.Plugin.AgentTesting/Runtime/AgentTestSyntheticConversationProbe.cs b/src/Plugins/BotSharp.Plugin.AgentTesting/Runtime/AgentTestSyntheticConversationProbe.cs new file mode 100644 index 000000000..3280f4576 --- /dev/null +++ b/src/Plugins/BotSharp.Plugin.AgentTesting/Runtime/AgentTestSyntheticConversationProbe.cs @@ -0,0 +1,31 @@ +using BotSharp.Abstraction.Conversations; + +namespace BotSharp.Plugin.AgentTesting.Runtime; + +/// +/// Tells BotSharp which conversations belong to a test run, so its per-user volume limits can stand +/// aside for them. +/// +/// Answers from -- the same registry that decides whether a +/// function call is intercepted -- rather than from the conversation's "test-set" tag. The tag is +/// written only once the conversation row exists, which is after the first message has already passed +/// through the rate limit hook, so a tag-based answer would still block the first turn of every case. +/// The registry entry is created before the conversation is opened at all. +/// +/// It is also the same source of truth as the mock seam, which matters: a conversation the harness +/// mocks tools for and a conversation the harness is exempt from rate limiting are, by construction, +/// the same set. Two independent notions of "is this a test" could disagree, and the disagreement +/// would show up as either real traffic escaping the limits or tests being blocked by them. +/// +public class AgentTestSyntheticConversationProbe : ISyntheticConversationProbe +{ + private readonly IAgentTestRunRegistry _registry; + + public AgentTestSyntheticConversationProbe(IAgentTestRunRegistry registry) + { + _registry = registry; + } + + public bool IsSynthetic(string conversationId) + => !string.IsNullOrEmpty(conversationId) && _registry.TryGet(conversationId) != null; +} diff --git a/src/Plugins/BotSharp.Plugin.AgentTesting/Services/AgentTestCaseRunner.cs b/src/Plugins/BotSharp.Plugin.AgentTesting/Services/AgentTestCaseRunner.cs index bbe3d7070..ad36f2641 100644 --- a/src/Plugins/BotSharp.Plugin.AgentTesting/Services/AgentTestCaseRunner.cs +++ b/src/Plugins/BotSharp.Plugin.AgentTesting/Services/AgentTestCaseRunner.cs @@ -7,16 +7,30 @@ public class AgentTestCaseRunner : ICaseRunner { private readonly IAgentTestRunRegistry _registry; private readonly IAgentConversationDriver _driver; + private readonly IAgentTestJudge? _judge; + private readonly ITokenStatistics? _tokens; private readonly ILogger _logger; public AgentTestCaseRunner( IAgentTestRunRegistry registry, IAgentConversationDriver driver, - ILogger logger) + ILogger logger, + IAgentTestJudge? judge = null, + ITokenStatistics? tokens = null) { _registry = registry; _driver = driver; _logger = logger; + // Optional so the orchestration tests can build a runner without one. ITokenStatistics is + // registered scoped, and AgentTestRunQueue.ScopedCaseRunner opens a fresh scope per case, so + // in production this is the same instance the conversation's completion provider reports + // into -- which is what makes the delta below attributable to this case alone. + _tokens = tokens; + // Optional so the orchestration tests can construct a runner without a vendor. A null judge + // is not a silent skip: EvaluateAsync turns an llmJudge assertion into the same Error as an + // unreachable vendor, because a case whose quality assertion was never scored has an unknown + // verdict, not a passing one. + _judge = judge; } public async Task RunAsync( @@ -37,7 +51,11 @@ public async Task RunAsync( // timeout) still say which model they were meant to run under -- a result that cannot // be attributed to a model is useless in a comparison run. Provider = model?.Provider, - Model = model?.Model + Model = model?.Model, + // Stamped up front like Provider/Model: aggregating routing accuracy separately from + // agent pass rate has to work off the result rows alone, including the rows produced by + // the early returns below. + CaseType = testCase.CaseType }; // Turns.SelectMany(...).Concat(caseAssertions).All(a => a.Passed) is vacuously true on an @@ -51,6 +69,15 @@ public async Task RunAsync( return result; } + // Which agent the conversation opens on. BotSharp dispatches on that agent's own type + // (ConversationService.SendMessage: a Routing agent goes through RoutingService.InstructLoop + // and can hand off, everything else through InstructDirect and cannot), so this one value is + // what decides whether the router is part of what the case measures. The suite's agent stays + // the default, so every case authored before EntryAgentId existed behaves exactly as before. + var entryAgentId = string.IsNullOrWhiteSpace(testCase.EntryAgentId) + ? suite.AgentId + : testCase.EntryAgentId!; + var active = new ActiveTestRun { ConversationId = conversationId, @@ -63,6 +90,14 @@ public async Task RunAsync( }; var stopwatch = Stopwatch.StartNew(); + + // Read as a delta, not an absolute: a scope that outlived an earlier case would otherwise + // have this case billed for that one's tokens too. On a fresh scope the baseline is zero and + // the delta is simply the final reading. + var tokensBefore = _tokens?.Total ?? 0; + var costBefore = _tokens?.AccumulatedCost ?? 0f; + long modelDurationMs = 0; + using var timeout = CancellationTokenSource.CreateLinkedTokenSource(ct); timeout.CancelAfter(TimeSpan.FromSeconds(Math.Max(1, suite.CaseTimeoutSeconds))); @@ -118,7 +153,31 @@ async Task AwaitOrHandOffAsync(Task driverTask) _registry.Register(active); try { - await _driver.PrepareAsync(conversationId, suite.AgentId, testCase.InitialStates); + await _driver.PrepareAsync(conversationId, entryAgentId, testCase.InitialStates); + + // Authored history goes in before the canary and before any turn, so the model sees it + // as the conversation's opening context. A short count means the write silently did + // nothing (see IAgentConversationDriver.InjectHistoryAsync) -- that has to be an Error, + // because a case running without the context it was written around would otherwise + // report an ordinary pass or fail about a scenario that never existed. + var injectedHistory = await AwaitOrHandOffAsync( + _driver.InjectHistoryAsync(conversationId, entryAgentId, testCase.History)); + + if (injectedHistory != testCase.History.Count) + { + result.Status = AgentTestStatus.Error; + result.Error = $"only {injectedHistory} of {testCase.History.Count} history messages " + + "could be written to the conversation, so this case would have run " + + "without the context it was written around"; + return result; + } + + // Authored history is not something the agent under test did, so it must not appear in + // the chain. Read once here and used as the offset for both the per-turn slices and the + // case-level chain, so the exclusion cannot drift between them. + var historyAssistantMessages = testCase.History.Count == 0 + ? 0 + : (await _driver.ReadAssistantAgentSequenceAsync(conversationId)).Count; // Prove the seam is live first. A dead seam means mocking silently does nothing and // real tools execute, so this has to happen before a single user message is sent. @@ -129,7 +188,7 @@ async Task AwaitOrHandOffAsync(Task driverTask) // active.CanaryIntercepted would look stricter but checks the same fact twice, and it // would stop a fake driver from unit-testing the orchestration at all -- a fake driver // has no ActiveTestRun and can never set that flag. - if (!await AwaitOrHandOffAsync(_driver.RunCanaryAsync(conversationId, suite.AgentId, timeout.Token))) + if (!await AwaitOrHandOffAsync(_driver.RunCanaryAsync(conversationId, entryAgentId, timeout.Token))) { result.Status = AgentTestStatus.Error; result.Error = "the mock seam is not live: IFunctionExecutorProvider was not consulted. " @@ -138,33 +197,57 @@ async Task AwaitOrHandOffAsync(Task driverTask) return result; } + // How many assistant messages the chain has already accounted for. The driver hands + // back the whole conversation on every read -- that is what the dialog store holds -- + // and only the runner knows where each turn started, so the per-turn slice is taken + // here. + var consumedAssistantMessages = historyAssistantMessages; + var fatalStop = false; foreach (var turn in testCase.Turns.OrderBy(t => t.Index)) { if (fatalStop) break; active.CurrentTurnIndex = turn.Index; + // Timed around the agent call ONLY. The case's own DurationMs also covers the + // canary, the mock lookups and the conversation reads, and on a fast case that + // overhead is a big enough share to move a latency percentile. + var turnTimer = Stopwatch.StartNew(); var output = await AwaitOrHandOffAsync( - _driver.SendAsync(conversationId, suite.AgentId, turn.UserMessage, timeout.Token)); + _driver.SendAsync(conversationId, entryAgentId, turn.UserMessage, timeout.Token)); + turnTimer.Stop(); + modelDurationMs += turnTimer.ElapsedMilliseconds; + + var agentSequence = await _driver.ReadAssistantAgentSequenceAsync(conversationId); + var turnChain = CollapseConsecutiveRepeats(agentSequence.Skip(consumedAssistantMessages)); + consumedAssistantMessages = agentSequence.Count; var turnResult = new TurnResult { Index = turn.Index, UserMessage = turn.UserMessage, - Output = output + Output = output, + ModelDurationMs = turnTimer.ElapsedMilliseconds, + // Names, not hops: a result is read by a person, and the ids are only needed + // while an assertion is being evaluated. + AgentChain = turnChain.Select(hop => hop.Name).ToList() }; + // This turn's slice, not the whole conversation: routedToAgent reads the chain's + // last entry, and a turn-level context carrying the conversation's chain would let a + // turn that produced no answer at all inherit the previous turn's agent and pass an + // assertion about routing that never happened. var turnContext = new AssertionContext { Output = output, ToolCalls = active.ObservedCalls.Where(c => c.TurnIndex == turn.Index).ToList(), States = await _driver.ReadStatesAsync(conversationId), - RoutedToAgent = await _driver.ReadRoutedAgentNameAsync(conversationId) + AgentChain = turnChain }; foreach (var assertion in turn.Assertions) { - var evaluated = AssertionEvaluator.Evaluate(assertion, turnContext); + var evaluated = await EvaluateAsync(assertion, turnContext, suite, timeout.Token); turnResult.Assertions.Add(evaluated); if (!evaluated.Passed && assertion.Fatal) { @@ -175,17 +258,21 @@ async Task AwaitOrHandOffAsync(Task driverTask) result.Turns.Add(turnResult); } + var caseChain = CollapseConsecutiveRepeats( + (await _driver.ReadAssistantAgentSequenceAsync(conversationId)).Skip(historyAssistantMessages)); + result.AgentChain = caseChain.Select(hop => hop.Name).ToList(); + var finalContext = new AssertionContext { Output = result.Turns.LastOrDefault()?.Output, ToolCalls = active.ObservedCalls, States = await _driver.ReadStatesAsync(conversationId), - RoutedToAgent = await _driver.ReadRoutedAgentNameAsync(conversationId) + AgentChain = caseChain }; foreach (var assertion in testCase.Assertions) { - result.Assertions.Add(AssertionEvaluator.Evaluate(assertion, finalContext)); + result.Assertions.Add(await EvaluateAsync(assertion, finalContext, suite, timeout.Token)); } result.ObservedToolCalls = active.ObservedCalls.ToList(); @@ -215,6 +302,18 @@ async Task AwaitOrHandOffAsync(Task driverTask) result.Status = AgentTestStatus.Cancelled; result.ObservedToolCalls = active.ObservedCalls.ToList(); } + catch (AgentTestJudgeUnavailableException ex) + { + // The judge never reached a verdict. That is Error, not Failed: a vendor timeout or an + // unconfigured judge model says nothing about the agent under test, and reporting it as + // a failing assertion would make provider noise indistinguishable from an agent + // regression. Logged at warning, not error -- this is a configuration or vendor + // condition, not a crash in the harness. + _logger.LogWarning(ex, "Agent test case {CaseId} could not be judged.", testCase.Id); + result.Status = AgentTestStatus.Error; + result.Error = ex.Message; + result.ObservedToolCalls = active.ObservedCalls.ToList(); + } catch (Exception ex) { // Reaches here for any OperationCanceledException that was neither our own timeout nor @@ -238,11 +337,82 @@ async Task AwaitOrHandOffAsync(Task driverTask) } stopwatch.Stop(); result.DurationMs = stopwatch.ElapsedMilliseconds; + result.ModelDurationMs = modelDurationMs; + + // In the finally block so a timed-out or crashed case still reports what it spent. A run + // that fell over having burned the budget is exactly the run whose cost matters. + result.TotalTokens = Math.Max(0, (_tokens?.Total ?? 0) - tokensBefore); + result.Cost = Math.Max(0, (_tokens?.AccumulatedCost ?? 0f) - costBefore); + + if (result.TotalTokens == 0 && result.Turns.Count > 0 && _tokens != null) + { + // Every turn calls the model, so zero tokens across completed turns means this + // runner's ITokenStatistics is not the instance the completion provider reported + // into. Logged rather than failed: usage accounting being wrong says nothing about + // whether the agent behaved, and failing the case would report a metering problem as + // an agent regression. + _logger.LogWarning( + "Agent test case {CaseId} completed {TurnCount} turn(s) but measured zero tokens; " + + "token and cost figures for this run are not trustworthy.", + testCase.Id, result.Turns.Count); + } } return result; } + /// + /// Consecutive duplicates removed, so ["A", "A", "B", "A"] becomes ["A", "B", "A"]. Only + /// consecutive ones: a conversation that really went A -> B -> A did visit A twice, and + /// flattening that to ["A", "B"] would hide the return hop from an ordered agentChain assertion. + /// One agent emitting several messages in a row is not a hand-off and does collapse. + /// + private static List CollapseConsecutiveRepeats(IEnumerable hops) + { + var chain = new List(); + foreach (var hop in hops) + { + // Compared by id, not name: two agents can share a display name, and collapsing those + // together would hide a real hand-off. + if (chain.Count == 0 || !string.Equals(chain[^1].Id, hop.Id, StringComparison.OrdinalIgnoreCase)) + { + chain.Add(hop); + } + } + + return chain; + } + + /// + /// Evaluates one assertion. Everything except llmJudge goes to the pure, synchronous + /// ; llmJudge needs a model call, so it goes to + /// instead. Keeping the split here rather than inside the evaluator + /// is what lets every other assertion type stay reproducible and I/O-free. + /// + /// Any propagates deliberately: the caller turns + /// it into a case-level Error. Swallowing it into a failing assertion would report a vendor + /// problem as an agent regression. + /// + private async Task EvaluateAsync( + TestAssertion assertion, + AssertionContext context, + AgentTestSuite suite, + CancellationToken ct) + { + if (!string.Equals(assertion.Type, AssertionTypes.LlmJudge, StringComparison.Ordinal)) + { + return AssertionEvaluator.Evaluate(assertion, context); + } + + if (_judge == null) + { + throw new AgentTestJudgeUnavailableException( + "no IAgentTestJudge is registered, so llmJudge assertions cannot be scored"); + } + + return await _judge.JudgeAsync(assertion, context, suite, ct); + } + /// /// A blocked tool call fails the case, as a synthetic case-level assertion. /// diff --git a/src/Plugins/BotSharp.Plugin.AgentTesting/Services/AgentTestRunExecutor.cs b/src/Plugins/BotSharp.Plugin.AgentTesting/Services/AgentTestRunExecutor.cs index 9faef9f05..6a293cee1 100644 --- a/src/Plugins/BotSharp.Plugin.AgentTesting/Services/AgentTestRunExecutor.cs +++ b/src/Plugins/BotSharp.Plugin.AgentTesting/Services/AgentTestRunExecutor.cs @@ -1,3 +1,5 @@ +using BotSharp.Abstraction.MLTasks; +using BotSharp.Abstraction.MLTasks.Settings; using BotSharp.Plugin.AgentTesting.Repositories; namespace BotSharp.Plugin.AgentTesting.Services; @@ -30,14 +32,20 @@ public class AgentTestRunExecutor { private readonly IAgentTestRepository _repo; private readonly ICaseRunner _caseRunner; + private readonly ILlmProviderService? _llmProviders; private readonly ILogger _logger; public AgentTestRunExecutor( IAgentTestRepository repo, ICaseRunner caseRunner, - ILogger logger) + ILogger logger, + ILlmProviderService? llmProviders = null) { _repo = repo; + // Optional so the run-orchestration tests can construct an executor without a provider + // registry. Without it the pricing snapshot is simply absent, which reads as "unknown" rather + // than as "free". + _llmProviders = llmProviders; _caseRunner = caseRunner; _logger = logger; } @@ -186,6 +194,7 @@ public async Task ExecuteAsync(string runId, CancellationToken ct) // weight in a grid keyed by model. Provider = model?.Provider, Model = model?.Model, + CaseType = testCase.CaseType, Status = AgentTestStatus.Error, Error = ex.Message }; @@ -204,6 +213,7 @@ public async Task ExecuteAsync(string runId, CancellationToken ct) run = await _repo.GetRunAsync(runId) ?? run; run.TotalCount++; + TallyRoutingAccuracy(run, result); switch (result.Status) { case AgentTestStatus.Passed: @@ -234,6 +244,159 @@ public async Task ExecuteAsync(string runId, CancellationToken ct) ? AgentTestStatus.Passed : AgentTestStatus.Failed; run.CompletedAt = DateTime.UtcNow; + + await SummarisePerformanceAsync(run); + await _repo.UpdateRunAsync(run); } + + /// + /// Fills in the run's per-model latency, token and cost figures, plus the pricing that produced + /// the cost. + /// + /// Once, at the end, reading the results back -- not accumulated per case like the counts are. + /// A percentile is not incrementally computable: it needs every value at once, and keeping the + /// whole list on the run document to update it in place would store the same numbers twice. + /// + private async Task SummarisePerformanceAsync(AgentTestRun run) + { + List results; + try + { + results = await _repo.ListCaseResultsAsync(run.Id); + } + catch (Exception ex) + { + // Reporting figures must never cost the run its terminal status: the case results are + // already stored and are the source of truth, so a failure here loses a summary, not + // data. + _logger.LogWarning(ex, "Could not summarise performance for agent test run {RunId}.", run.Id); + return; + } + + run.PerformanceSummaries = results + .GroupBy(r => (r.Provider, r.Model)) + .Select(group => BuildSummary(group.Key.Provider, group.Key.Model, group.ToList())) + .ToList(); + + run.ModelPricing = SnapshotPricing(run); + } + + private static PerformanceSummary BuildSummary( + string? provider, string? model, List results) + { + // Only cases that reached the model. An Error case that died before its first turn has a + // ModelDurationMs of zero, and letting those into the percentile makes a run that mostly + // crashed look like the fastest one on record. + var latencies = results + .Where(r => r.ModelDurationMs > 0) + .Select(r => r.ModelDurationMs) + .OrderBy(ms => ms) + .ToList(); + + return new PerformanceSummary + { + Provider = provider, + Model = model, + CaseCount = latencies.Count, + LatencyP50Ms = Percentile(latencies, 0.50), + LatencyP95Ms = Percentile(latencies, 0.95), + // Tokens and cost come from EVERY result, unlike latency: a case that errored still spent + // whatever it spent, and hiding that would understate the run's real cost. + TotalTokens = results.Sum(r => r.TotalTokens), + TotalCost = results.Sum(r => r.Cost) + }; + } + + /// + /// Nearest-rank percentile over an already-sorted list: the value at ceil(p * n) - 1, so P95 of + /// twenty samples is the 19th and P50 of an even count is the lower of the two middle values. + /// + /// Deliberately not interpolated. An interpolated P95 returns a duration no case actually took, + /// which is indefensible when someone asks which case was the slow one -- and with the handful of + /// cases a real suite starts with, interpolation invents most of the answer. + /// + private static long Percentile(List sorted, double percentile) + { + if (sorted.Count == 0) + { + return 0; + } + + var rank = (int)Math.Ceiling(percentile * sorted.Count) - 1; + return sorted[Math.Clamp(rank, 0, sorted.Count - 1)]; + } + + /// + /// The unit costs in force for each model this run swept. Without them a cost figure cannot be + /// compared with any other run's: a provider price change would show up as a cost regression with + /// nothing to point at. + /// + private List SnapshotPricing(AgentTestRun run) + { + if (_llmProviders == null || run.Models is not { Count: > 0 }) + { + // No models named means each agent ran on its own LlmConfig, which this method cannot + // resolve without reading every agent involved. Left empty rather than guessed. + return []; + } + + return run.Models + .Select(m => + { + LlmModelSetting? setting = null; + try + { + setting = _llmProviders.GetSetting(m.Provider, m.Model); + } + catch (Exception ex) + { + _logger.LogWarning(ex, + "Could not read pricing for {Provider}/{Model} while summarising run {RunId}.", + m.Provider, m.Model, run.Id); + } + + return new ModelPricingSnapshot + { + Provider = m.Provider, + Model = m.Model, + TextInputCost = setting?.Cost?.TextInputCost, + TextOutputCost = setting?.Cost?.TextOutputCost + }; + }) + .ToList(); + } + + /// + /// Folds one case result into the run's per-model routing accuracy. Only Routing cases count: + /// the evaluation framework gates routing accuracy separately from the agent pass rate, so + /// mixing an agent case into this figure would make the gate unreadable. + /// + /// Rows are keyed by (provider, model) and created on first sight, which keeps this correct for + /// a run that sweeps no models at all -- that produces the single (null, null) row. + /// + private static void TallyRoutingAccuracy(AgentTestRun run, AgentTestCaseResult result) + { + if (!string.Equals(result.CaseType, CaseTypes.Routing, StringComparison.OrdinalIgnoreCase)) + { + return; + } + + var row = run.RoutingAccuracies.FirstOrDefault(a => + string.Equals(a.Provider, result.Provider, StringComparison.OrdinalIgnoreCase) + && string.Equals(a.Model, result.Model, StringComparison.OrdinalIgnoreCase)); + + if (row == null) + { + row = new RoutingAccuracy { Provider = result.Provider, Model = result.Model }; + run.RoutingAccuracies.Add(row); + } + + row.CaseCount++; + if (string.Equals(result.Status, AgentTestStatus.Passed, StringComparison.Ordinal)) + { + row.PassedCount++; + } + } + } diff --git a/src/Plugins/BotSharp.Plugin.AgentTesting/Services/AssertionContext.cs b/src/Plugins/BotSharp.Plugin.AgentTesting/Services/AssertionContext.cs index 6234163e7..0f34f6541 100644 --- a/src/Plugins/BotSharp.Plugin.AgentTesting/Services/AssertionContext.cs +++ b/src/Plugins/BotSharp.Plugin.AgentTesting/Services/AssertionContext.cs @@ -9,5 +9,41 @@ public class AssertionContext public string? Output { get; set; } public IReadOnlyList ToolCalls { get; set; } = []; public IReadOnlyDictionary States { get; set; } = new Dictionary(); - public string? RoutedToAgent { get; set; } + + /// + /// Which agents answered, in order, consecutive repeats collapsed. Turn-level contexts carry only + /// that turn's slice; the case-level context carries the whole conversation. + /// + /// There is deliberately no separate "routed to agent" field: routedToAgent is this chain's last + /// entry, and two fields fed from one read are two things that can drift apart. + /// + public IReadOnlyList AgentChain { get; set; } = []; +} + +/// +/// One agent in a chain, carrying BOTH identifiers on purpose. +/// +/// An assertion has to accept either. The name is what a human reads and what the UI shows, but it +/// is also mutable -- renaming an agent would silently break every routing case asserting on it. The +/// id is stable but is a guid nobody recognises, and it is exactly what an author copies out of the +/// agent list, which is how the first real routing case came to assert an id against a name and could +/// never pass. +/// +/// Not persisted: stores names, because that is what a +/// person reads off a result. +/// +public class AgentChainHop +{ + public string Id { get; set; } = string.Empty; + + /// Falls back to the id when the agent cannot be loaded, so this is never blank. + public string Name { get; set; } = string.Empty; + + /// + /// Whether a token an author typed refers to this hop. Case-insensitive, and matches the id just + /// as readily as the name. + /// + public bool Matches(string token) + => string.Equals(Name, token, StringComparison.OrdinalIgnoreCase) + || string.Equals(Id, token, StringComparison.OrdinalIgnoreCase); } diff --git a/src/Plugins/BotSharp.Plugin.AgentTesting/Services/AssertionEvaluator.cs b/src/Plugins/BotSharp.Plugin.AgentTesting/Services/AssertionEvaluator.cs index 6041beb7c..9c3554eea 100644 --- a/src/Plugins/BotSharp.Plugin.AgentTesting/Services/AssertionEvaluator.cs +++ b/src/Plugins/BotSharp.Plugin.AgentTesting/Services/AssertionEvaluator.cs @@ -12,13 +12,14 @@ public static class AssertionTypes public const string ToolNotCalled = "toolNotCalled"; public const string StateEquals = "stateEquals"; public const string RoutedToAgent = "routedToAgent"; + public const string AgentChain = "agentChain"; public const string LlmJudge = "llmJudge"; /// /// Result-only. Never authored on a case and never evaluated -- AgentTestCaseRunner synthesises /// it when the mock seam blocked a tool, so the block surfaces in the ordinary assertion table /// instead of only in Observed Tool Calls. Deliberately absent from AssertionValidation's - /// Requirements map, which covers the eight authorable types. + /// Requirements map, which covers the nine authorable types. /// public const string NoBlockedTools = "noBlockedTools"; } @@ -177,28 +178,35 @@ public static AssertionResult Evaluate(TestAssertion assertion, AssertionContext break; case AssertionTypes.RoutedToAgent: - result.Actual = context.RoutedToAgent; + var lastHop = context.AgentChain.Count > 0 ? context.AgentChain[^1] : null; + result.Actual = lastHop?.Name; if (string.IsNullOrWhiteSpace(assertion.Expected)) { - // A null Expected compares equal to a null RoutedToAgent (e.g. the canary/no - // routing information case) -- a blank/omitted Expected must not read as - // "expect no routing," which verifies nothing and would always pass. + // A blank Expected would compare equal to "no agent answered", which verifies + // nothing and would always pass. result.Passed = false; - result.Message = "routedToAgent requires a non-empty 'expected' agent name"; + result.Message = "routedToAgent requires a non-empty 'expected' agent name or id"; break; } - result.Passed = string.Equals(context.RoutedToAgent, assertion.Expected, - StringComparison.OrdinalIgnoreCase); + // Either identifier is accepted -- see AgentChainHop.Matches. + result.Passed = lastHop?.Matches(assertion.Expected.Trim()) == true; if (!result.Passed) result.Message = "the conversation was handled by a different agent"; break; + case AssertionTypes.AgentChain: + EvaluateAgentChain(assertion, context, result); + break; + case AssertionTypes.LlmJudge: - // P2 will wire this to an IInstructService judge. P1 fails explicitly and never - // passes silently -- passing silently would show a case that verified nothing as - // green. + // Unreachable on the normal path: the runner routes llmJudge to IAgentTestJudge + // instead of here, because scoring it needs a model call and this method is a pure, + // synchronous, I/O-free function. Kept as a loud failure rather than removed, so + // that a caller who evaluates assertions without going through the runner gets a + // verdict it cannot mistake for a pass. Silently passing would show a case that + // verified nothing as green. result.Passed = false; - result.Message = "llmJudge is not available in P1"; + result.Message = "llmJudge must be evaluated through IAgentTestJudge, not AssertionEvaluator"; break; default: @@ -209,6 +217,125 @@ public static AssertionResult Evaluate(TestAssertion assertion, AssertionContext return result; } + + /// + /// Compares the agents that answered against an expected list. Complements routedToAgent rather + /// than replacing it: routedToAgent asks "who answered last", which cannot express a hand-off at + /// all -- in Entry -> A -> B only B is visible, and if control returns to the entry agent and it + /// emits the closing message, a correctly routed case reads as routed to the entry agent. + /// + /// is a comma-separated list of agent names. + /// selects the mode -- see -- + /// and defaults to Contains when omitted. + /// + private static void EvaluateAgentChain( + TestAssertion assertion, AssertionContext context, AssertionResult result) + { + result.Actual = string.Join(" -> ", context.AgentChain.Select(hop => hop.Name)); + + var expected = SplitAgentNames(assertion.Expected); + if (expected.Count == 0) + { + // An empty expected list is a subset of, and an ordered subsequence of, any chain, so + // both Contains and Ordered would pass vacuously; Exact would silently assert "no agent + // ever answered", which no author means to write. + result.Passed = false; + result.Message = "agentChain requires a non-empty comma-separated 'expected' agent list"; + return; + } + + // Deliberately not defaulted on a typo: falling back to Contains for an unrecognised mode + // would turn "orderd" into the loosest available check and quietly verify much less than the + // author asked for. + var mode = AgentChainModes.Normalize(assertion.Target); + if (mode == null) + { + result.Passed = false; + result.Message = "agentChain 'target' must be one of " + + string.Join(", ", AgentChainModes.All) + + " (or empty for " + AgentChainModes.Contains + "), not '" + + assertion.Target + "'"; + return; + } + + switch (mode) + { + case AgentChainModes.Exact: + result.Passed = context.AgentChain.Count == expected.Count + && context.AgentChain.Zip(expected).All(pair => pair.First.Matches(pair.Second)); + if (!result.Passed) result.Message = "the agent chain differs from the expected chain"; + break; + + case AgentChainModes.Ordered: + result.Passed = IsOrderedSubsequence(expected, context.AgentChain); + if (!result.Passed) result.Message = "the expected agents did not all appear, in that order"; + break; + + default: + var missing = expected + .Where(e => !context.AgentChain.Any(hop => hop.Matches(e))) + .ToList(); + result.Passed = missing.Count == 0; + if (!result.Passed) + { + result.Message = "the agent chain does not include " + string.Join(", ", missing); + } + break; + } + } + + /// + /// Whether every expected name appears in the chain in the given relative order, with other + /// agents allowed in between -- so ["Copilot", "WorkOrder"] matches + /// Copilot -> Diagnosis -> WorkOrder. Asserting the hand-offs an author cares about must not + /// require enumerating every agent the conversation happened to pass through; Exact is for that. + /// + private static bool IsOrderedSubsequence(List expected, IReadOnlyList chain) + { + var next = 0; + foreach (var hop in chain) + { + if (next < expected.Count && hop.Matches(expected[next])) + { + next++; + } + } + + return next == expected.Count; + } + + private static List SplitAgentNames(string? value) + => (value ?? string.Empty) + .Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries) + .ToList(); +} + +/// +/// How an agentChain assertion compares its expected list against the actual chain. +/// +/// Contains -- every expected agent appears somewhere, order ignored. The default, and the right +/// choice for "this agent must have been involved at all". +/// Ordered -- every expected agent appears, in that relative order, other agents allowed in +/// between. This is the hand-off assertion. +/// Exact -- the chain is precisely the expected list and nothing else. Strictest, and also how to +/// assert isolation: an Exact chain of one agent means nothing routed away. +/// +public static class AgentChainModes +{ + public const string Contains = "contains"; + public const string Ordered = "ordered"; + public const string Exact = "exact"; + + public static readonly string[] All = [Contains, Ordered, Exact]; + + /// + /// Canonical mode for any casing, with blank meaning ; null for an + /// unrecognised value, so the caller rejects it instead of guessing. + /// + public static string? Normalize(string? value) + => string.IsNullOrWhiteSpace(value) + ? Contains + : All.FirstOrDefault(m => string.Equals(m, value.Trim(), StringComparison.OrdinalIgnoreCase)); } /// @@ -224,7 +351,7 @@ public static class AssertionValidation { private enum RequiredField { Expected, Target } - // One row per AssertionTypes constant -- eight total. + // One row per authorable AssertionTypes constant -- nine total. private static readonly Dictionary Requirements = new(StringComparer.Ordinal) { [AssertionTypes.OutputContains] = RequiredField.Expected, @@ -234,6 +361,7 @@ private enum RequiredField { Expected, Target } [AssertionTypes.ToolNotCalled] = RequiredField.Target, [AssertionTypes.StateEquals] = RequiredField.Target, [AssertionTypes.RoutedToAgent] = RequiredField.Expected, + [AssertionTypes.AgentChain] = RequiredField.Expected, [AssertionTypes.LlmJudge] = RequiredField.Expected, }; diff --git a/src/Plugins/BotSharp.Plugin.AgentTesting/Services/BotSharpAgentConversationDriver.cs b/src/Plugins/BotSharp.Plugin.AgentTesting/Services/BotSharpAgentConversationDriver.cs index 75ff79711..ee3198c97 100644 --- a/src/Plugins/BotSharp.Plugin.AgentTesting/Services/BotSharpAgentConversationDriver.cs +++ b/src/Plugins/BotSharp.Plugin.AgentTesting/Services/BotSharpAgentConversationDriver.cs @@ -1,5 +1,6 @@ using BotSharp.Abstraction.Agents; using BotSharp.Abstraction.Agents.Enums; +using BotSharp.Abstraction.Conversations.Enums; using BotSharp.Abstraction.Models; using BotSharp.Abstraction.Repositories; using BotSharp.Abstraction.Routing; @@ -37,6 +38,9 @@ public class BotSharpAgentConversationDriver : IAgentConversationDriver // see AgentTestRunQueue.ScopedCaseRunner), so exactly one conversation ever passes through it. private bool _conversationTagged; + /// See . Scoped to one case, like the driver itself. + private readonly Dictionary _agentNameCache = new(StringComparer.Ordinal); + public BotSharpAgentConversationDriver( IConversationService conversations, IRoutingService routing, @@ -78,6 +82,50 @@ public async Task PrepareAsync(string conversationId, string agentId, IReadOnlyL await _conversations.SetConversationId(conversationId, states); } + public async Task InjectHistoryAsync( + string conversationId, string agentId, IReadOnlyList history) + { + if (history.Count == 0) + { + return 0; + } + + // The conversation's dialog document has to exist before AppendConversationDialogs can push + // into it -- see the interface note. Created through the same call SendMessage itself uses, + // so the row a history-bearing case runs against is identical to the row it would have got + // anyway, rather than one this method invented. + await _conversations.GetConversationRecordOrCreateNew(agentId); + + var elements = history + .Select(message => new DialogElement + { + MetaData = new DialogMetaData + { + // Already normalised and rejected at save time; falling back to user here just + // avoids writing a role BotSharp would not recognise if that ever slipped through. + Role = HistoryRoles.Normalize(message.Role) ?? AgentRole.User, + AgentId = agentId, + MessageId = Guid.NewGuid().ToString(), + MessageType = MessageTypeName.Plain, + CreatedTime = DateTime.UtcNow + }, + Content = message.Content + }) + .ToList(); + + await _repository.AppendConversationDialogs(conversationId, elements); + + // Read back and count only the messages this call generated, matched by their own message + // ids. Counting every dialog in the conversation would also count anything else that got + // there, which is exactly the confusion this verification exists to avoid. + var written = elements + .Select(e => e.MetaData!.MessageId) + .ToHashSet(StringComparer.Ordinal); + + var stored = await _repository.GetConversationDialogs(conversationId); + return stored.Count(d => d.MetaData?.MessageId != null && written.Contains(d.MetaData.MessageId)); + } + public async Task SendAsync(string conversationId, string agentId, string userMessage, CancellationToken ct) { // Fail fast only if the case had already timed out before we ever got here (e.g. a prior @@ -181,18 +229,70 @@ public async Task RunCanaryAsync(string conversationId, string agentId, Ca return Task.FromResult>(result); } - public async Task ReadRoutedAgentNameAsync(string conversationId) + public async Task> ReadAssistantAgentSequenceAsync(string conversationId) { var dialogs = await _repository.GetConversationDialogs(conversationId); - var lastAssistantDialog = dialogs.LastOrDefault(d => d.MetaData?.Role == AgentRole.Assistant); - var agentId = lastAssistantDialog?.MetaData?.AgentId; - if (string.IsNullOrEmpty(agentId)) + var sequence = new List(); + foreach (var dialog in dialogs) { - return null; + if (dialog.MetaData?.Role != AgentRole.Assistant) + { + continue; + } + + var agentId = dialog.MetaData?.AgentId; + if (string.IsNullOrEmpty(agentId)) + { + // An assistant message with no agent attribution cannot be placed in the chain. + // Skipped rather than recorded as a blank hop, which would make an agentChain + // assertion fail for a reason the author has no way to act on. + continue; + } + + sequence.Add(new AgentChainHop + { + Id = agentId, + Name = await ResolveAgentNameAsync(agentId) + }); + } + + return sequence; + } + + /// + /// Agent id to display name, cached for the lifetime of this driver -- which is one case, since + /// the driver is resolved from the per-case DI scope AgentTestRunQueue.ScopedCaseRunner opens. + /// The chain is re-read after every turn and a long conversation revisits the same few agents + /// many times, so without the cache one case would issue GetAgent once per assistant message per + /// turn. + /// + /// Falls back to the id when the agent cannot be loaded (deleted since the conversation ran, or + /// a permission-scoped lookup): a chain entry that names something is far more actionable than a + /// silently dropped hop, and an assertion against it fails with a message the author can read. + /// + private async Task ResolveAgentNameAsync(string agentId) + { + if (_agentNameCache.TryGetValue(agentId, out var cached)) + { + return cached; + } + + string name; + try + { + var agent = await _agents.GetAgent(agentId); + name = string.IsNullOrWhiteSpace(agent?.Name) ? agentId : agent!.Name; + } + catch (Exception ex) + { + _logger.LogWarning(ex, + "Could not resolve agent {AgentId} while building an agent test chain; using the id.", + agentId); + name = agentId; } - var agent = await _agents.GetAgent(agentId); - return agent?.Name; + _agentNameCache[agentId] = name; + return name; } } diff --git a/src/Plugins/BotSharp.Plugin.AgentTesting/Services/CaseScope.cs b/src/Plugins/BotSharp.Plugin.AgentTesting/Services/CaseScope.cs new file mode 100644 index 000000000..faee58382 --- /dev/null +++ b/src/Plugins/BotSharp.Plugin.AgentTesting/Services/CaseScope.cs @@ -0,0 +1,172 @@ +namespace BotSharp.Plugin.AgentTesting.Services; + +/// +/// Works out which cases a given change actually needs to run. +/// +/// A model or prompt change does not justify running everything: the cost is real and mostly buys +/// nothing, since a change to one agent cannot affect a case that never touches it. But the reverse +/// error is far worse -- a case wrongly left out reports nothing at all, and "not run" is +/// indistinguishable from "passed" once the numbers are in a report. So every rule here resolves +/// towards including, and the one thing the caller must never be able to do is quietly shrink the +/// scope without it showing up in the excluded list. +/// +/// A pure function, like : the same (case, suite agent, query) always +/// yields the same decision with no I/O, which is what makes the rules exhaustively testable rather +/// than something to be argued about. +/// +public static class CaseScope +{ + /// + /// The agents a case exercises, as ids. + /// + /// An authored wins. Otherwise it falls back to the + /// case's entry agent, which is already known and is definitionally involved -- so an Agent case + /// is correctly picked up by a change to the agent it runs against without anyone maintaining a + /// list. + /// + /// That fallback is exactly right for an Agent case and only a starting point for a Routing case, + /// where the entry agent is the router and the agents that matter are downstream of it. Those + /// cannot be derived from the case definition -- they are only visible once it has run -- which is + /// what authoring InvolvedAgents is for. + /// + public static IReadOnlyList InvolvedAgentIds(AgentTestCase testCase, string? suiteAgentId) + { + if (testCase.InvolvedAgents.Count > 0) + { + return testCase.InvolvedAgents + .Where(id => !string.IsNullOrWhiteSpace(id)) + .Select(id => id.Trim()) + .ToList(); + } + + var entry = string.IsNullOrWhiteSpace(testCase.EntryAgentId) ? suiteAgentId : testCase.EntryAgentId; + return string.IsNullOrWhiteSpace(entry) ? [] : [entry.Trim()]; + } + + /// + /// Whether this case belongs in the scope, and which rule decided it. Rules are applied in the + /// order below and the first match wins. + /// + public static ScopeDecision Decide(AgentTestCase testCase, string? suiteAgentId, ScopeQuery query) + { + var involved = InvolvedAgentIds(testCase, suiteAgentId); + var batch = CaseBatches.Effective(testCase); + + // Before the rules: a disabled case is skipped by the executor, so calling it "in scope" + // would overstate coverage by exactly the cases nobody is running. Reported as excluded with + // its own reason so a disabled cross-cutting safety case stands out rather than blending into + // the cases a change genuinely cannot affect. + if (!testCase.Enabled) + { + return new ScopeDecision(false, ScopeReasons.Disabled, involved, batch); + } + + // A separate axis from the rules below: batches exist to run in order and stop early, so + // narrowing to one batch is scheduling rather than scoping. Null means every batch. + if (query.Batch is { } wanted && batch != wanted) + { + return new ScopeDecision(false, ScopeReasons.OtherBatch, involved, batch); + } + + // Rule 1. Cross-cutting cases run in every scope, whatever changed. The whole point of + // narrowing is to skip cases a change cannot affect, and "this change cannot affect safety" + // is precisely the claim not to accept without checking. + if (testCase.CrossCutting) + { + return new ScopeDecision(true, ScopeReasons.CrossCutting, involved, batch); + } + + // Rule 2. A platform-wide change -- foundation model, provider swap, infrastructure -- turns + // narrowing off entirely, because there is no agent it demonstrably does not touch. + if (query.FullPlatform) + { + return new ScopeDecision(true, ScopeReasons.FullPlatform, involved, batch); + } + + // Rule 3. The case touches at least one of the changed agents. + if (involved.Any(id => query.TargetAgentIds.Any( + target => string.Equals(id, target, StringComparison.OrdinalIgnoreCase)))) + { + return new ScopeDecision(true, ScopeReasons.TargetAgent, involved, batch); + } + + // Rule 4. Nothing matched. Note this is only reachable when the case HAS a known involved + // set: with neither an authored list nor an entry agent the set is empty, which is handled + // below rather than falling through to an exclusion. + if (involved.Count == 0) + { + // Fail open. An unknown involved set means the harness cannot show the change does not + // affect this case, and a wrongly excluded case is silent -- it produces no result to + // notice. Running one case more than necessary costs tokens; skipping one hides a + // regression. + return new ScopeDecision(true, ScopeReasons.UnknownAgents, involved, batch); + } + + return new ScopeDecision(false, ScopeReasons.NotInvolved, involved, batch); + } +} + +/// What the caller says changed. +public class ScopeQuery +{ + /// Agent ids the change touches. Ignored when is set. + public IReadOnlyList TargetAgentIds { get; set; } = []; + + /// A platform-wide change: narrowing is switched off and every enabled case is in. + public bool FullPlatform { get; set; } + + /// Narrow to one batch; null covers all of them. + public int? Batch { get; set; } +} + +/// Why one case is in or out. +public class ScopeDecision +{ + public ScopeDecision(bool included, string reason, IReadOnlyList involvedAgentIds, int batch) + { + Included = included; + Reason = reason; + InvolvedAgentIds = involvedAgentIds; + Batch = batch; + } + + public bool Included { get; } + + /// See . Always populated, for excluded cases too. + public string Reason { get; } + + /// The set the decision was actually made against, authored or derived. + public IReadOnlyList InvolvedAgentIds { get; } + + /// The effective batch, after the priority and cross-cutting derivation. + public int Batch { get; } +} + +/// +/// Which rule decided a case. Reported per case rather than only as a count, because a scope nobody +/// can explain is a scope nobody can review -- and reviewing it is the only defence against a change +/// being signed off against a set of cases that quietly excluded the interesting one. +/// +public static class ScopeReasons +{ + /// Included: cross-cutting, so it runs in every scope. + public const string CrossCutting = "crossCutting"; + + /// Included: the change is platform-wide, so narrowing is off. + public const string FullPlatform = "fullPlatform"; + + /// Included: the case exercises one of the changed agents. + public const string TargetAgent = "targetAgent"; + + /// Included: no involved agents are known, so it cannot be shown to be unaffected. + public const string UnknownAgents = "unknownAgents"; + + /// Excluded: the case does not touch any changed agent. + public const string NotInvolved = "notInvolved"; + + /// Excluded: the case is disabled, so no run would execute it. + public const string Disabled = "disabled"; + + /// Excluded: the case belongs to a different batch than the one asked for. + public const string OtherBatch = "otherBatch"; +} diff --git a/src/Plugins/BotSharp.Plugin.AgentTesting/Services/IAgentConversationDriver.cs b/src/Plugins/BotSharp.Plugin.AgentTesting/Services/IAgentConversationDriver.cs index 4226daebb..de6fce7cb 100644 --- a/src/Plugins/BotSharp.Plugin.AgentTesting/Services/IAgentConversationDriver.cs +++ b/src/Plugins/BotSharp.Plugin.AgentTesting/Services/IAgentConversationDriver.cs @@ -9,6 +9,19 @@ public interface IAgentConversationDriver { Task PrepareAsync(string conversationId, string agentId, IReadOnlyList initialStates); + /// + /// Writes authored history into the conversation before any turn runs, and returns how many of + /// those messages are actually readable back out of the store. + /// + /// The return value is the point. IBotSharpRepository.AppendConversationDialogs is an UpdateOne + /// with no upsert, so it silently does nothing when the conversation's dialog document does not + /// exist yet -- and PrepareAsync deliberately does not create it (the row is created by the + /// first SendMessage). A case whose history vanished would run against no context at all and + /// still report Passed, so the caller compares this count against what it asked for. + /// + Task InjectHistoryAsync( + string conversationId, string agentId, IReadOnlyList history); + /// Drives one turn and returns that turn's output text. Task SendAsync(string conversationId, string agentId, string userMessage, CancellationToken ct); @@ -17,5 +30,16 @@ public interface IAgentConversationDriver Task> ReadStatesAsync(string conversationId); - Task ReadRoutedAgentNameAsync(string conversationId); + /// + /// The agent behind every assistant message so far, in dialog order, NOT de-duplicated and NOT + /// sliced per turn -- the caller does both, because only it knows where each turn began. + /// + /// Replaces an earlier ReadRoutedAgentNameAsync that returned just the last assistant message's + /// agent. That was the only routing signal available and it could not describe a hand-off: for + /// Entry -> A -> B it reported B alone, and when control returned to the entry agent and that + /// agent emitted the closing message it reported the entry agent, failing a correctly routed + /// case. Returning the sequence lets the runner derive both the last agent and the chain from + /// one read, so they cannot disagree, and costs no extra round trip. + /// + Task> ReadAssistantAgentSequenceAsync(string conversationId); } diff --git a/src/Plugins/BotSharp.Plugin.AgentTesting/Services/IAgentTestJudge.cs b/src/Plugins/BotSharp.Plugin.AgentTesting/Services/IAgentTestJudge.cs new file mode 100644 index 000000000..903dfc772 --- /dev/null +++ b/src/Plugins/BotSharp.Plugin.AgentTesting/Services/IAgentTestJudge.cs @@ -0,0 +1,55 @@ +namespace BotSharp.Plugin.AgentTesting.Services; + +/// +/// Raised when the judge could not reach a verdict at all: no judge model is configured on the +/// suite, its provider is not registered, the vendor call failed, or the reply could not be read as +/// a score. +/// +/// Deliberately an exception rather than a failing . "The judge could +/// not decide" is not "the agent regressed" -- folding the two together would let a vendor timeout, +/// a rate limit or a malformed reply read as an agent defect, which is exactly the Failed/Error +/// confusion the runner keeps apart everywhere else. turns this +/// into a case-level Error carrying this message. +/// +public class AgentTestJudgeUnavailableException : Exception +{ + public AgentTestJudgeUnavailableException(string message) : base(message) + { + } + + public AgentTestJudgeUnavailableException(string message, Exception inner) : base(message, inner) + { + } +} + +/// +/// Scores one llmJudge assertion with a model. +/// +/// This is the one assertion type that is NOT a pure function, which is why it lives here instead of +/// in : that class is a pure, synchronous, I/O-free function, and the +/// runner depends on that purity for reproducible verdicts. A model call is neither pure nor +/// synchronous, so llmJudge is evaluated in a separate pass and the reproducible assertions stay +/// reproducible. +/// +/// What reaches the vendor is deliberately narrow: the fixed rubric, the criterion the case author +/// wrote () and the agent's reply text. Tool arguments, tool +/// results, conversation state and the user's own messages are NOT sent. That is the same boundary +/// draws, with one unavoidable difference: judging the quality of a +/// reply requires sending that reply, and an agent's reply can contain PII (phone numbers, +/// addresses, tenant names). Callers need to know that. It also means a criterion has to be +/// self-contained -- "the reply asks for a work order number" works, "the reply answers the user's +/// question" does not, because the judge never sees the question. +/// +public interface IAgentTestJudge +{ + /// + /// Scores against . Throws + /// when no verdict could be reached; a returned + /// result is always a real verdict, passing or failing. + /// + Task JudgeAsync( + TestAssertion assertion, + AssertionContext context, + AgentTestSuite suite, + CancellationToken ct); +} diff --git a/src/Plugins/BotSharp.Plugin.AgentTesting/Services/LlmAgentTestJudge.cs b/src/Plugins/BotSharp.Plugin.AgentTesting/Services/LlmAgentTestJudge.cs new file mode 100644 index 000000000..5525df4c1 --- /dev/null +++ b/src/Plugins/BotSharp.Plugin.AgentTesting/Services/LlmAgentTestJudge.cs @@ -0,0 +1,236 @@ +using System.Text.Json; +using BotSharp.Abstraction.Agents.Enums; +using BotSharp.Abstraction.MLTasks; + +namespace BotSharp.Plugin.AgentTesting.Services; + +/// +/// The model-backed . Contract, boundary and the reason this is not part +/// of all live on the interface. +/// +/// Extends the model's answer no trust: a blank reply, a non-JSON reply, malformed JSON or a score +/// outside the 1-5 scale are all rejected as "no verdict" rather than coerced into a pass or a fail. +/// A judge that ignored the rubric has not graded anything, and reading pass/fail out of that is +/// reading meaning into noise. +/// +public class LlmAgentTestJudge : IAgentTestJudge +{ + /// + /// Pass mark when a case does not set . 4/5 is the bar the + /// evaluation framework uses for its own human quality score, and the rubric below is worded to + /// match that framework's 1-5 definitions -- so a model score and a human score mean the same + /// thing on the same scale, and the two can be compared or swapped later. + /// + public const double DefaultMinScore = 4; + + private const double MinValidScore = 1; + private const double MaxValidScore = 5; + + private readonly IServiceProvider _services; + private readonly ILogger _logger; + + public LlmAgentTestJudge(IServiceProvider services, ILogger logger) + { + _services = services; + _logger = logger; + } + + public async Task JudgeAsync( + TestAssertion assertion, + AssertionContext context, + AgentTestSuite suite, + CancellationToken ct) + { + var criterion = assertion.Expected; + if (string.IsNullOrWhiteSpace(criterion)) + { + // AssertionValidation already rejects this at case create/update. Repeated here because + // a case saved before that rule existed would otherwise reach the vendor with an empty + // criterion and come back with a meaningless score. + throw new AgentTestJudgeUnavailableException( + "this llmJudge assertion has no 'expected' criterion to judge against"); + } + + // No silent default. BotSharp's own InstructService falls back to openai/gpt-4o when a + // provider and model are not given; inheriting that here would score cases with a model + // nobody chose, and the run would look conclusive. A missing judge model is an Error with a + // message naming the fix. + if (string.IsNullOrWhiteSpace(suite.JudgeProvider) || string.IsNullOrWhiteSpace(suite.JudgeModel)) + { + throw new AgentTestJudgeUnavailableException( + "this suite has no judge model configured, so llmJudge assertions cannot be scored. " + + "Set the suite's judgeProvider and judgeModel, or remove the llmJudge assertion."); + } + + if (string.IsNullOrWhiteSpace(context.Output)) + { + // Not a failing verdict: the agent produced no text at all, so there is nothing for the + // judge to grade. Whatever went wrong upstream is the real finding, and the other + // assertions on this case will say so far more usefully than a fabricated score would. + throw new AgentTestJudgeUnavailableException( + "the agent produced no reply text, so there is nothing for llmJudge to score"); + } + + ct.ThrowIfCancellationRequested(); + + // Resolved straight from DI rather than through BotSharp.Core's CompletionProvider helper, + // for the same two reasons LlmCaseSegmenter gives: this plugin references only + // BotSharp.Abstraction, and that helper also writes provider/model into the ambient + // conversation state -- which here would leak the JUDGE's model into the conversation under + // test and change what the agent itself runs on. + var completion = _services.GetServices() + .FirstOrDefault(x => string.Equals(x.Provider, suite.JudgeProvider, StringComparison.OrdinalIgnoreCase)); + + if (completion == null) + { + throw new AgentTestJudgeUnavailableException( + $"no chat completion provider is registered for judge provider '{suite.JudgeProvider}'"); + } + + completion.SetModelName(suite.JudgeModel); + + var promptAgent = new Agent + { + Id = Guid.Empty.ToString(), + Name = "AgentTestJudge", + Instruction = BuildInstruction() + }; + + string raw; + try + { + var response = await completion.GetChatCompletions( + promptAgent, + [new RoleDialogModel(AgentRole.User, BuildPrompt(criterion, context.Output))]); + + raw = response?.Content ?? string.Empty; + } + catch (Exception ex) + { + // A vendor timeout, a rate limit, a bad key. None of these say anything about the agent + // under test, so none of them may surface as a failing assertion. + throw new AgentTestJudgeUnavailableException( + $"the judge model call failed: {ex.Message}", ex); + } + + var verdict = ParseVerdict(raw); + var threshold = assertion.MinScore ?? DefaultMinScore; + + var result = new AssertionResult + { + Type = assertion.Type, + Target = assertion.Target, + Expected = criterion, + Actual = verdict.Score.ToString("0.#"), + Score = verdict.Score, + Passed = verdict.Score >= threshold + }; + + // The reason is recorded whether it passed or failed: a pass at exactly the threshold is + // worth being able to read afterwards, and a judge's stated reason is the only way to tell + // "graded correctly" from "graded plausibly but wrongly". + result.Message = string.IsNullOrWhiteSpace(verdict.Reason) + ? $"judged {verdict.Score:0.#}/5 against a threshold of {threshold:0.#}" + : $"judged {verdict.Score:0.#}/5 against a threshold of {threshold:0.#}: {verdict.Reason}"; + + _logger.LogInformation( + "llmJudge scored {Score}/5 (threshold {Threshold}) using {Provider}/{Model}.", + verdict.Score, threshold, suite.JudgeProvider, suite.JudgeModel); + + return result; + } + + /// + /// Parses and validates the judge's reply. Public and static so it can be unit-tested without a + /// vendor: everything that can realistically go wrong with a model's answer is decided here. + /// + public static JudgeVerdict ParseVerdict(string raw) + { + var json = ExtractJson(raw); + if (json == null) + { + throw new AgentTestJudgeUnavailableException( + $"the judge model did not return JSON. First 200 chars: {Truncate(raw, 200)}"); + } + + JudgeVerdict? verdict; + try + { + verdict = JsonSerializer.Deserialize(json, new JsonSerializerOptions + { + PropertyNameCaseInsensitive = true + }); + } + catch (JsonException ex) + { + throw new AgentTestJudgeUnavailableException( + $"the judge model returned malformed JSON: {ex.Message}"); + } + + if (verdict == null) + { + throw new AgentTestJudgeUnavailableException("the judge model returned an empty verdict"); + } + + if (verdict.Score < MinValidScore || verdict.Score > MaxValidScore || double.IsNaN(verdict.Score)) + { + // A score off the scale means the rubric was not followed. Clamping it would silently + // turn "did not grade" into a grade. + throw new AgentTestJudgeUnavailableException( + $"the judge model returned {verdict.Score:0.#}, outside the 1-5 scale it was given"); + } + + return verdict; + } + + private static string? ExtractJson(string raw) + { + var start = raw.IndexOf('{'); + var end = raw.LastIndexOf('}'); + return start >= 0 && end > start ? raw[start..(end + 1)] : null; + } + + private static string Truncate(string text, int max) + => text.Length <= max ? text : text[..max].TrimEnd() + "..."; + + /// + /// The 1-5 definitions are worded to match the evaluation framework's human scoring scale, so + /// that a model score and a human score are directly comparable. Changing them decouples the two. + /// + private static string BuildInstruction() => + """ + You grade one reply produced by a customer-service AI agent, as part of an automated + regression test. + + You are given exactly one CRITERION and the agent's REPLY. Judge only how well the reply + satisfies that criterion. Do not grade tone, length, formatting or style unless the criterion + asks about them. Do not judge whether the underlying business facts are true -- you have no + way to check them and other assertions already cover that. + + Score on this scale: + 5 - clearly satisfies the criterion, no substantive problem + 4 - satisfies the criterion, only minor problems + 3 - broadly usable, but with an obvious gap + 2 - noticeable problems that affect use + 1 - unacceptable + + Reply with JSON only. No prose, no code fence, no explanation outside the JSON: + {"score": , "reason": ""} + """; + + private static string BuildPrompt(string criterion, string output) => + $""" + CRITERION: + {criterion} + + REPLY: + {output} + """; +} + +/// One judge verdict, as returned by the model. +public class JudgeVerdict +{ + public double Score { get; set; } + public string? Reason { get; set; } +} diff --git a/tests/BotSharp.Core.UnitTests/AgentTesting/AgentChainAssertionTests.cs b/tests/BotSharp.Core.UnitTests/AgentTesting/AgentChainAssertionTests.cs new file mode 100644 index 000000000..4772510e9 --- /dev/null +++ b/tests/BotSharp.Core.UnitTests/AgentTesting/AgentChainAssertionTests.cs @@ -0,0 +1,282 @@ +using System.Collections.Generic; +using System.Linq; +using BotSharp.Plugin.AgentTesting.Services; +using BotSharp.Plugin.AgentTesting.Models; +using Xunit; + +namespace BotSharp.Core.UnitTests.AgentTesting; + +/// +/// agentChain is the only assertion that can describe a hand-off, so its three modes are the whole +/// vocabulary available for asserting hand-offs. routedToAgent answers "who spoke last", which +/// cannot express a hand-off at all: for Entry -> A -> B it sees only B, and when control returns to +/// the entry agent and that agent closes the conversation it sees the entry agent, so a correctly +/// routed case reads as a routing failure. +/// +/// Two properties matter more than the happy paths and are pinned first: an empty expected list and +/// an unrecognised mode both have to FAIL rather than pass vacuously or fall back to the loosest +/// check, because either would show a case that verified nothing as green. +/// +public class AgentChainAssertionTests +{ + /// + /// Names only, ids left blank. The id-matching path has its own tests at the end of the file, + /// where the distinction between the two identifiers is the point. + /// + private static AssertionContext Context(params string[] chain) => new() + { + AgentChain = chain.Select(name => new AgentChainHop { Id = string.Empty, Name = name }).ToList() + }; + + private static AssertionResult Evaluate(string? expected, string? mode, params string[] chain) + => AssertionEvaluator.Evaluate( + new TestAssertion { Type = AssertionTypes.AgentChain, Expected = expected, Target = mode }, + Context(chain)); + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + [InlineData(",")] + [InlineData(" , , ")] + public void An_empty_expected_list_fails_instead_of_passing_vacuously(string? expected) + { + // An empty list is a subset of, and an ordered subsequence of, every chain, so Contains and + // Ordered would both pass while checking nothing. Exact would assert "no agent ever + // answered", which no author writes on purpose. Note that "," and " , , " parse to an empty + // list too, so the guard has to be on the parsed result rather than on the raw string. + var result = Evaluate(expected, mode: null, "Copilot"); + + Assert.False(result.Passed); + Assert.Contains("non-empty", result.Message); + } + + [Theory] + [InlineData("orderd")] + [InlineData("subsequence")] + [InlineData("any")] + public void An_unrecognised_mode_fails_rather_than_falling_back_to_the_loosest_check(string mode) + { + // Defaulting a typo to Contains would silently downgrade "these agents, in this order" to + // "these agents, in any order" -- the assertion still goes green, having verified much less + // than it says. The message names the accepted values so the author can fix it. + var result = Evaluate("Copilot, Work Order Creator", mode, "Work Order Creator", "Copilot"); + + Assert.False(result.Passed); + Assert.Contains(AgentChainModes.Ordered, result.Message); + Assert.Contains(mode, result.Message); + } + + [Fact] + public void A_blank_mode_means_contains() + { + // The common case is "this agent must have been involved", so the default is the loosest + // mode -- but only when the author left it blank, never as a recovery from a typo. + var result = Evaluate("Work Order Creator", mode: null, "Copilot", "Work Order Creator"); + + Assert.True(result.Passed); + } + + [Fact] + public void Contains_ignores_order() + { + var result = Evaluate( + "Work Order Creator, Copilot", AgentChainModes.Contains, "Copilot", "Work Order Creator"); + + Assert.True(result.Passed); + } + + [Fact] + public void Contains_names_the_agents_that_are_missing() + { + // Which agent was absent is the whole diagnostic. "the chain does not match" would leave the + // author diffing two lists by eye. + var result = Evaluate( + "Copilot, Diagnosis, Work Order Creator", AgentChainModes.Contains, "Copilot", "Work Order Creator"); + + Assert.False(result.Passed); + Assert.Contains("Diagnosis", result.Message); + Assert.DoesNotContain("Copilot", result.Message); + } + + [Fact] + public void Ordered_allows_other_agents_in_between() + { + // Asserting the hand-offs an author cares about must not require enumerating every agent the + // conversation happened to pass through -- that is what Exact is for. + var result = Evaluate( + "Copilot, Work Order Creator", AgentChainModes.Ordered, + "Copilot", "Diagnosis", "Work Order Creator"); + + Assert.True(result.Passed); + } + + [Fact] + public void Ordered_rejects_the_reverse_order() + { + // The one thing Contains cannot see. Both agents are present, so Contains would pass; the + // hand-off went the wrong way round. + var chain = new[] { "Work Order Creator", "Copilot" }; + + Assert.True(Evaluate("Copilot, Work Order Creator", AgentChainModes.Contains, chain).Passed); + Assert.False(Evaluate("Copilot, Work Order Creator", AgentChainModes.Ordered, chain).Passed); + } + + [Fact] + public void Ordered_matches_a_repeated_agent_against_a_return_hop() + { + // The runner collapses only CONSECUTIVE repeats, so a genuine return hop survives in the + // chain -- and this is the assertion that reads it. Requiring Copilot twice must match + // Copilot -> WO -> Copilot and not Copilot -> WO. + Assert.True(Evaluate( + "Copilot, Work Order Creator, Copilot", AgentChainModes.Ordered, + "Copilot", "Work Order Creator", "Copilot").Passed); + + Assert.False(Evaluate( + "Copilot, Work Order Creator, Copilot", AgentChainModes.Ordered, + "Copilot", "Work Order Creator").Passed); + } + + [Fact] + public void Exact_rejects_an_extra_agent_that_ordered_would_allow() + { + var chain = new[] { "Copilot", "Diagnosis", "Work Order Creator" }; + + Assert.True(Evaluate("Copilot, Work Order Creator", AgentChainModes.Ordered, chain).Passed); + Assert.False(Evaluate("Copilot, Work Order Creator", AgentChainModes.Exact, chain).Passed); + } + + [Fact] + public void Exact_on_a_single_agent_is_how_isolation_is_asserted() + { + // An Agent case is supposed to measure one agent with the router out of the picture, but + // route_to_agent stays on the allow list during it, so a leaf agent that routes onward turns + // the case into a multi-agent one with nothing to show it. An Exact chain of one agent is the + // assertion that catches that. + Assert.True(Evaluate("Work Order Creator", AgentChainModes.Exact, "Work Order Creator").Passed); + Assert.False(Evaluate( + "Work Order Creator", AgentChainModes.Exact, "Work Order Creator", "Diagnosis").Passed); + } + + [Fact] + public void Agent_names_are_matched_case_insensitively_and_trimmed() + { + // Authors type agent names by hand and copy them out of the UI, so casing and stray spaces + // around the commas are not a reason to fail a case. Mirrors routedToAgent, which has always + // compared OrdinalIgnoreCase. + var result = Evaluate( + " copilot , WORK ORDER CREATOR ", AgentChainModes.Ordered, "Copilot", "Work Order Creator"); + + Assert.True(result.Passed); + } + + [Fact] + public void The_actual_chain_is_reported_in_a_readable_form() + { + // The chain is what the author has to reason about when the assertion fails, and a bare + // list would be rendered by the UI as an opaque blob. Arrow-joined mirrors how the hand-off + // reads in the conversation. + var result = Evaluate("Nobody", AgentChainModes.Contains, "Copilot", "Work Order Creator"); + + Assert.Equal("Copilot -> Work Order Creator", result.Actual); + } + + [Fact] + public void An_empty_chain_fails_every_mode_but_never_throws() + { + // A case whose agent never answered (or one that errored before any turn ran) leaves the + // chain empty. That has to be an ordinary failing assertion, not an exception that turns the + // case into an infrastructure Error and hides the real problem. + foreach (var mode in AgentChainModes.All) + { + var result = Evaluate("Copilot", mode); + + Assert.False(result.Passed); + Assert.Equal(string.Empty, result.Actual); + } + } + + [Fact] + public void Save_time_validation_requires_the_expected_list() + { + // The same guard as at evaluation time, but at case create/update, so a chain assertion with + // nothing to compare against is rejected while its author is still looking at it. + Assert.NotNull(AssertionValidation.Validate( + new TestAssertion { Type = AssertionTypes.AgentChain, Target = AgentChainModes.Ordered })); + + Assert.Null(AssertionValidation.Validate( + new TestAssertion { Type = AssertionTypes.AgentChain, Expected = "Copilot" })); + } + + private static AssertionContext ChainOf(params string[] idAndName) => new() + { + // Pairs, flattened: ("id1", "Name 1", "id2", "Name 2"). + AgentChain = Enumerable.Range(0, idAndName.Length / 2) + .Select(i => new AgentChainHop { Id = idAndName[i * 2], Name = idAndName[i * 2 + 1] }) + .ToList() + }; + + [Fact] + public void An_expected_agent_may_be_given_as_an_id_instead_of_a_name() + { + // The id is exactly what an author copies out of the agent list, and the first real routing + // case did precisely that: it asserted routedToAgent against a guid while the chain reported + // a display name, so it could never pass no matter what the agent did. + var context = ChainOf("0fe3905d-75f1-4e2e-8e54-ec3d33d6b6f0", "WO Cancellation"); + + var byId = AssertionEvaluator.Evaluate( + new TestAssertion + { + Type = AssertionTypes.RoutedToAgent, + Expected = "0fe3905d-75f1-4e2e-8e54-ec3d33d6b6f0" + }, + context); + + var byName = AssertionEvaluator.Evaluate( + new TestAssertion { Type = AssertionTypes.RoutedToAgent, Expected = "WO Cancellation" }, + context); + + Assert.True(byId.Passed); + Assert.True(byName.Passed); + + // Either way the reported actual is the readable name, never the guid. + Assert.Equal("WO Cancellation", byId.Actual); + } + + [Fact] + public void An_agent_chain_accepts_ids_names_and_a_mixture_of_both() + { + // Authors paste whichever identifier is in front of them, and a chain listing several agents + // is the most likely place to end up with a mixture. + var context = ChainOf( + "2cd4b805-7078-4405-87e9-2ec9aadf8a11", "Lessen Copilot", + "0fe3905d-75f1-4e2e-8e54-ec3d33d6b6f0", "WO Cancellation"); + + var result = AssertionEvaluator.Evaluate( + new TestAssertion + { + Type = AssertionTypes.AgentChain, + Target = AgentChainModes.Ordered, + Expected = "2cd4b805-7078-4405-87e9-2ec9aadf8a11, WO Cancellation" + }, + context); + + Assert.True(result.Passed); + } + + [Fact] + public void A_wrong_id_still_fails() + { + // Accepting either identifier must not become accepting anything: an id belonging to some + // other agent has to fail exactly as a wrong name does. + var result = AssertionEvaluator.Evaluate( + new TestAssertion + { + Type = AssertionTypes.RoutedToAgent, + Expected = "11111111-2222-3333-4444-555555555555" + }, + ChainOf("0fe3905d-75f1-4e2e-8e54-ec3d33d6b6f0", "WO Cancellation")); + + Assert.False(result.Passed); + } +} diff --git a/tests/BotSharp.Core.UnitTests/AgentTesting/AgentTestCaseRunnerTests.cs b/tests/BotSharp.Core.UnitTests/AgentTesting/AgentTestCaseRunnerTests.cs index 373aecf63..529c8b5d9 100644 --- a/tests/BotSharp.Core.UnitTests/AgentTesting/AgentTestCaseRunnerTests.cs +++ b/tests/BotSharp.Core.UnitTests/AgentTesting/AgentTestCaseRunnerTests.cs @@ -10,6 +10,8 @@ using BotSharp.Plugin.AgentTesting.Services; using BotSharp.Plugin.AgentTesting.Models; using Xunit; +using BotSharp.Abstraction.Conversations; +using BotSharp.Abstraction.Conversations.Models; namespace BotSharp.Core.UnitTests.AgentTesting; @@ -32,6 +34,51 @@ private sealed class FakeDriver : IAgentConversationDriver public string? RoutedAgent { get; set; } public TimeSpan SendDelay { get; set; } = TimeSpan.Zero; + /// + /// The agents that answer each turn, one entry per SendAsync in call order. Lets a test model + /// a hand-off inside a single turn (["Copilot", "WorkOrder"]), one agent answering several + /// times in a row (["A", "A"]), or a turn that produced no answer at all ([]) -- none of + /// which can express. A turn with no entry here is answered by + /// RoutedAgent, which is what leaves the pre-existing single-agent tests untouched. + /// + public List AgentsPerTurn { get; } = []; + + /// + /// The accumulated assistant-message sequence, exactly as the real driver reads it back out + /// of the dialog store: never sliced per turn and never de-duplicated, because slicing and + /// collapsing are the runner's job and that is what these tests are checking. + /// + private readonly List _agentSequence = []; + + /// Everything InjectHistoryAsync was handed, in order. + public List InjectedHistory { get; } = []; + + /// + /// Overrides what InjectHistoryAsync reports as written, to simulate the real failure mode: + /// AppendConversationDialogs is an UpdateOne with no upsert, so it silently writes nothing + /// when the dialog document is missing. Null reports every message as written. + /// + public int? HistoryWriteCount { get; set; } + + /// + /// Agent the authored assistant history is attributed to. Set it to something the turns do + /// NOT use to prove the history is excluded from the chain rather than merely collapsed into + /// the first turn's entry. + /// + public string? HistoryAgentName { get; set; } + + /// + /// Runs at the start of each SendAsync. Lets a test move an external counter -- the token + /// meter -- during the case rather than only before or after it, which is the only way to + /// exercise the delta. + /// + public Action? OnSend { get; set; } + + /// Which agent each call was made against, so a test can prove which one won. + public string? PreparedAgentId { get; private set; } + public string? CanaryAgentId { get; private set; } + public List SentAgentIds { get; } = []; + // Lets the empty-Turns guard test assert that the seam was never touched at all, not merely // that no message was sent. public bool PrepareCalled { get; private set; } @@ -56,6 +103,7 @@ private sealed class FakeDriver : IAgentConversationDriver public Task PrepareAsync(string conversationId, string agentId, IReadOnlyList initialStates) { PrepareCalled = true; + PreparedAgentId = agentId; return Task.CompletedTask; } @@ -70,6 +118,18 @@ public Task PrepareAsync(string conversationId, string agentId, IReadOnlyList SendAsync(string conversationId, string agentId, string userMessage, CancellationToken ct) { Sent.Add(userMessage); + SentAgentIds.Add(agentId); + OnSend?.Invoke(); + + // Appended before any of the failure simulations below, like the real thing: BotSharp + // has already written the assistant dialogs by the time a later step throws. + string[] turnAgents = AgentsPerTurn.Count >= Sent.Count + ? AgentsPerTurn[Sent.Count - 1] + : RoutedAgent == null ? [] : [RoutedAgent]; + // Id mirrors the name here, because the runner collapses consecutive hops BY ID: giving + // every agent the same blank id would collapse a genuine hand-off away. + _agentSequence.AddRange( + turnAgents.Select(name => new AgentChainHop { Id = name, Name = name })); if (BlockedOnFirstSend.Count > 0 && Sent.Count == 1) { @@ -114,23 +174,62 @@ public Task PrepareAsync(string conversationId, string agentId, IReadOnlyList 0 ? Replies.Dequeue() : string.Empty; } + public Task InjectHistoryAsync( + string conversationId, string agentId, IReadOnlyList history) + { + InjectedHistory.AddRange(history); + + // Mirrors the real driver: an authored assistant message becomes a stored assistant + // dialog, so it turns up in the sequence and the runner has to exclude it from the chain. + // A fake that skipped this would let a broken offset pass. + foreach (var message in history) + { + if (string.Equals(message.Role, HistoryRoles.Assistant, StringComparison.OrdinalIgnoreCase)) + { + var name = HistoryAgentName ?? agentId; + _agentSequence.Add(new AgentChainHop { Id = name, Name = name }); + } + } + + return Task.FromResult(HistoryWriteCount ?? history.Count); + } + public Task RunCanaryAsync(string conversationId, string agentId, CancellationToken ct) { CanaryCalled = true; + CanaryAgentId = agentId; return Task.FromResult(CanaryResult); } public Task> ReadStatesAsync(string conversationId) => Task.FromResult>(States); - public Task ReadRoutedAgentNameAsync(string conversationId) - => Task.FromResult(RoutedAgent); + public Task> ReadAssistantAgentSequenceAsync(string conversationId) + => Task.FromResult>(_agentSequence.ToList()); + } + + /// + /// Reports whatever it is told to. Only Total and AccumulatedCost are read by the runner -- the + /// rest of ITokenStatistics exists for the completion providers that feed it. + /// + private sealed class FakeTokens : ITokenStatistics + { + public long Total { get; set; } + public float AccumulatedCost { get; set; } + public float Cost => AccumulatedCost; + + public Task AddToken(TokenStatsModel stats, RoleDialogModel message) => Task.CompletedTask; + public void PrintStatistics() { } + public void StartTimer() { } + public void StopTimer() { } } - private static AgentTestCaseRunner Build(FakeDriver driver, out AgentTestRunRegistry registry) + private static AgentTestCaseRunner Build( + FakeDriver driver, out AgentTestRunRegistry registry, ITokenStatistics? tokens = null) { registry = new AgentTestRunRegistry(); - return new AgentTestCaseRunner(registry, driver, NullLogger.Instance); + return new AgentTestCaseRunner( + registry, driver, NullLogger.Instance, judge: null, tokens: tokens); } private static AgentTestSuite Suite(int timeoutSeconds = 120) => new() @@ -557,4 +656,397 @@ public void Log( Entries.Add((logLevel, exception, formatter(state, exception))); } } + + [Fact] + public async Task The_cases_entry_agent_overrides_the_suites_on_every_driver_call() + { + // This one value decides whether the router is part of what the case measures, because + // BotSharp dispatches on the agent's own type (ConversationService.SendMessage: Routing -> + // InstructLoop and can hand off, anything else -> InstructDirect and cannot). It has to win + // on all three calls -- a case that prepares and sends against the entry agent but runs its + // canary against the suite's would prove the seam live on the wrong conversation shape. + var driver = new FakeDriver { RoutedAgent = "Work Order Creator" }; + driver.Replies.Enqueue("done"); + var runner = Build(driver, out _); + + await runner.RunAsync(Suite(), new AgentTestCase + { + Id = "case-1", + Name = "c", + EntryAgentId = "copilot-entry", + Turns = [new TestTurn { Index = 0, UserMessage = "a" }] + }, "run-1", model: null, CancellationToken.None); + + Assert.Equal("copilot-entry", driver.PreparedAgentId); + Assert.Equal("copilot-entry", driver.CanaryAgentId); + Assert.Equal(["copilot-entry"], driver.SentAgentIds); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public async Task A_case_with_no_entry_agent_still_uses_the_suites(string? entryAgentId) + { + // Every case authored before EntryAgentId existed deserialises with it null, so the fallback + // is what keeps those cases running against the same agent they always did. Blank strings go + // the same way: a UI that sends "" for an untouched field must not silently retarget a case + // at an agent id of "". + var driver = new FakeDriver(); + driver.Replies.Enqueue("done"); + var runner = Build(driver, out _); + + await runner.RunAsync(Suite(), new AgentTestCase + { + Id = "case-1", + Name = "c", + EntryAgentId = entryAgentId, + Turns = [new TestTurn { Index = 0, UserMessage = "a" }] + }, "run-1", model: null, CancellationToken.None); + + Assert.Equal("agent-1", driver.PreparedAgentId); + Assert.Equal(["agent-1"], driver.SentAgentIds); + } + + [Fact] + public async Task The_agent_chain_collapses_consecutive_repeats_but_keeps_a_return_hop() + { + // A -> A is one agent sending two messages, not a hand-off, so it collapses. The second A is + // a real return hop and must survive: flattening the chain to distinct agents would make an + // ordered agentChain assertion unable to see the conversation come back. + var driver = new FakeDriver(); + driver.AgentsPerTurn.Add(["Copilot", "Copilot", "Work Order Creator", "Copilot"]); + driver.Replies.Enqueue("done"); + var runner = Build(driver, out _); + + var result = await runner.RunAsync(Suite(), new AgentTestCase + { + Id = "case-1", + Name = "c", + Turns = [new TestTurn { Index = 0, UserMessage = "a" }] + }, "run-1", model: null, CancellationToken.None); + + Assert.Equal(["Copilot", "Work Order Creator", "Copilot"], result.AgentChain); + } + + [Fact] + public async Task Each_turns_chain_is_only_that_turns_slice() + { + // The driver hands back the whole conversation every time, so a turn's chain is the runner's + // own slice. The distinction is load-bearing here: turn 2's chain is ["Work Order Creator"] + // even though the case-level chain collapses that agent together with turn 1's trailing + // entry, so a per-turn chain cannot be derived from the case-level one. + var driver = new FakeDriver(); + driver.AgentsPerTurn.Add(["Copilot", "Work Order Creator"]); + driver.AgentsPerTurn.Add(["Work Order Creator"]); + driver.Replies.Enqueue("one"); + driver.Replies.Enqueue("two"); + var runner = Build(driver, out _); + + var result = await runner.RunAsync(Suite(), new AgentTestCase + { + Id = "case-1", + Name = "c", + Turns = + [ + new TestTurn { Index = 0, UserMessage = "a" }, + new TestTurn { Index = 1, UserMessage = "b" } + ] + }, "run-1", model: null, CancellationToken.None); + + Assert.Equal(["Copilot", "Work Order Creator"], result.Turns[0].AgentChain); + Assert.Equal(["Work Order Creator"], result.Turns[1].AgentChain); + Assert.Equal(["Copilot", "Work Order Creator"], result.AgentChain); + } + + [Fact] + public async Task A_turn_that_produced_no_answer_routes_to_nobody() + { + // Turn-level routedToAgent reads that turn's own last agent, not the conversation's. Were it + // the conversation's, this second turn would inherit turn 1's agent and pass an assertion + // about a turn that never routed anywhere at all. + var driver = new FakeDriver(); + driver.AgentsPerTurn.Add(["Work Order Creator"]); + driver.AgentsPerTurn.Add([]); + driver.Replies.Enqueue("one"); + driver.Replies.Enqueue("two"); + var runner = Build(driver, out _); + + var result = await runner.RunAsync(Suite(), new AgentTestCase + { + Id = "case-1", + Name = "c", + Turns = + [ + new TestTurn + { + Index = 0, + UserMessage = "a", + Assertions = + [ + new TestAssertion { Type = AssertionTypes.RoutedToAgent, Expected = "Work Order Creator" } + ] + }, + new TestTurn + { + Index = 1, + UserMessage = "b", + Assertions = + [ + new TestAssertion { Type = AssertionTypes.RoutedToAgent, Expected = "Work Order Creator" } + ] + } + ] + }, "run-1", model: null, CancellationToken.None); + + Assert.True(result.Turns[0].Assertions[0].Passed); + Assert.False(result.Turns[1].Assertions[0].Passed); + Assert.Empty(result.Turns[1].AgentChain); + } + + [Fact] + public async Task An_agent_chain_assertion_sees_a_hand_off_that_routed_to_agent_cannot() + { + // The reason agentChain exists. Control reaches the work order agent and comes back, so the + // last agent to speak is the entry agent -- routedToAgent reports Copilot and a correctly + // routed case looks like a routing failure. The chain still shows the hand-off happened. + var driver = new FakeDriver(); + driver.AgentsPerTurn.Add(["Copilot", "Work Order Creator", "Copilot"]); + driver.Replies.Enqueue("done"); + var runner = Build(driver, out _); + + var result = await runner.RunAsync(Suite(), new AgentTestCase + { + Id = "case-1", + Name = "c", + Turns = [new TestTurn { Index = 0, UserMessage = "a" }], + Assertions = + [ + new TestAssertion { Type = AssertionTypes.RoutedToAgent, Expected = "Work Order Creator" }, + new TestAssertion + { + Type = AssertionTypes.AgentChain, + Target = AgentChainModes.Ordered, + Expected = "Copilot, Work Order Creator" + } + ] + }, "run-1", model: null, CancellationToken.None); + + Assert.False(result.Assertions[0].Passed); + Assert.Equal("Copilot", result.Assertions[0].Actual); + Assert.True(result.Assertions[1].Passed); + } + + [Fact] + public async Task The_case_type_is_stamped_on_the_result_even_when_the_case_never_ran() + { + // Routing accuracy is aggregated from the result rows, so a row that cannot say what type of + // case it came from would silently drop out of the figure. The no-turns guard returns before + // the driver is touched at all, which is exactly the path most likely to forget the stamp. + var driver = new FakeDriver(); + var runner = Build(driver, out _); + + var result = await runner.RunAsync(Suite(), new AgentTestCase + { + Id = "case-1", + Name = "c", + CaseType = CaseTypes.Routing, + Turns = [] + }, "run-1", model: null, CancellationToken.None); + + Assert.Equal(AgentTestStatus.Error, result.Status); + Assert.Equal(CaseTypes.Routing, result.CaseType); + Assert.False(driver.PrepareCalled); + } + + [Fact] + public async Task Authored_history_is_written_before_the_first_turn_is_sent() + { + // The whole point is that the model sees the exchange as the conversation's opening context. + // Written after the first turn it would be context for the second turn onwards, which is a + // different scenario from the one the author wrote. + var driver = new FakeDriver { RoutedAgent = "Work Order Creator" }; + driver.Replies.Enqueue("done"); + var runner = Build(driver, out _); + + var result = await runner.RunAsync(Suite(), new AgentTestCase + { + Id = "case-1", + Name = "c", + History = + [ + new TestHistoryMessage { Role = HistoryRoles.User, Content = "my fridge is leaking" }, + new TestHistoryMessage { Role = HistoryRoles.Assistant, Content = "I raised work order B123." } + ], + Turns = [new TestTurn { Index = 0, UserMessage = "when is someone coming?" }] + }, "run-1", model: null, CancellationToken.None); + + Assert.Equal(AgentTestStatus.Passed, result.Status); + Assert.Equal(2, driver.InjectedHistory.Count); + Assert.Equal("my fridge is leaking", driver.InjectedHistory[0].Content); + Assert.Equal(["when is someone coming?"], driver.Sent); + } + + [Fact] + public async Task A_history_write_that_silently_did_nothing_errors_the_case() + { + // AppendConversationDialogs is an UpdateOne with no upsert, so a missing dialog document + // makes it a no-op that reports success. Left unchecked, the case would run with no context + // at all and report an ordinary pass or fail about a scenario that never existed -- the same + // "executed nothing, reports green" family as the canary and the no-turns guard. + var driver = new FakeDriver { HistoryWriteCount = 1 }; + driver.Replies.Enqueue("done"); + var runner = Build(driver, out _); + + var result = await runner.RunAsync(Suite(), new AgentTestCase + { + Id = "case-1", + Name = "c", + History = + [ + new TestHistoryMessage { Role = HistoryRoles.User, Content = "a" }, + new TestHistoryMessage { Role = HistoryRoles.Assistant, Content = "b" } + ], + Turns = [new TestTurn { Index = 0, UserMessage = "hi" }] + }, "run-1", model: null, CancellationToken.None); + + Assert.Equal(AgentTestStatus.Error, result.Status); + Assert.Contains("1 of 2 history messages", result.Error); + // And it stopped there rather than running the case anyway. + Assert.Empty(driver.Sent); + } + + [Fact] + public async Task Authored_history_is_excluded_from_the_agent_chain() + { + // Authored history is not something the agent under test did. Counting it would break the + // one assertion that most needs to be trustworthy: an exact chain of a single agent is how a + // case asserts nothing routed away, and a fabricated preamble in the chain would fail it for + // a reason the author never caused. + var driver = new FakeDriver { HistoryAgentName = "Copilot" }; + driver.AgentsPerTurn.Add(["Work Order Creator"]); + driver.Replies.Enqueue("done"); + var runner = Build(driver, out _); + + var result = await runner.RunAsync(Suite(), new AgentTestCase + { + Id = "case-1", + Name = "c", + History = [new TestHistoryMessage { Role = HistoryRoles.Assistant, Content = "earlier answer" }], + Turns = [new TestTurn { Index = 0, UserMessage = "hi" }], + Assertions = + [ + new TestAssertion + { + Type = AssertionTypes.AgentChain, + Target = AgentChainModes.Exact, + Expected = "Work Order Creator" + } + ] + }, "run-1", model: null, CancellationToken.None); + + Assert.Equal(["Work Order Creator"], result.AgentChain); + Assert.Equal(["Work Order Creator"], result.Turns[0].AgentChain); + Assert.True(result.Assertions[0].Passed); + } + + [Fact] + public async Task Token_usage_is_recorded_as_a_delta_not_an_absolute_reading() + { + // ITokenStatistics is scoped and the queue opens a scope per case, so on that path the + // baseline is zero. But a scope that outlived an earlier case would otherwise have this case + // billed for that one's tokens too, and a cost figure that charges the wrong case is worse + // than none. + var driver = new FakeDriver(); + driver.Replies.Enqueue("done"); + var tokens = new FakeTokens { Total = 500, AccumulatedCost = 0.05f }; + var runner = Build(driver, out _, tokens); + + // Simulates the conversation consuming 300 tokens during the case. + driver.OnSend = () => { tokens.Total = 800; tokens.AccumulatedCost = 0.08f; }; + + var result = await runner.RunAsync(Suite(), new AgentTestCase + { + Id = "case-1", + Name = "c", + Turns = [new TestTurn { Index = 0, UserMessage = "a" }] + }, "run-1", model: null, CancellationToken.None); + + Assert.Equal(300, result.TotalTokens); + Assert.Equal(0.03, result.Cost, precision: 4); + } + + [Fact] + public async Task Usage_is_still_recorded_when_the_case_times_out() + { + // A run that fell over having burned the budget is exactly the run whose cost matters, so the + // reading happens in the finally block rather than on the success path. + var driver = new FakeDriver { SendDelay = TimeSpan.FromSeconds(5) }; + var tokens = new FakeTokens(); + var runner = Build(driver, out _, tokens); + driver.OnSend = () => { tokens.Total = 120; tokens.AccumulatedCost = 0.01f; }; + + var result = await runner.RunAsync(Suite(timeoutSeconds: 1), new AgentTestCase + { + Id = "case-1", + Name = "c", + Turns = [new TestTurn { Index = 0, UserMessage = "a" }] + }, "run-1", model: null, CancellationToken.None); + + Assert.Equal(AgentTestStatus.Error, result.Status); + Assert.Equal(120, result.TotalTokens); + } + + [Fact] + public async Task Model_duration_covers_the_agent_call_and_not_the_harness() + { + // A latency gate read against the case's whole wall clock also measures the canary, the mock + // lookups and the conversation reads. On a fast case that overhead is a large enough share to + // move a percentile, so the two figures are kept apart. + var driver = new FakeDriver { SendDelay = TimeSpan.FromMilliseconds(120) }; + driver.Replies.Enqueue("one"); + driver.Replies.Enqueue("two"); + var runner = Build(driver, out _); + + var result = await runner.RunAsync(Suite(), new AgentTestCase + { + Id = "case-1", + Name = "c", + Turns = + [ + new TestTurn { Index = 0, UserMessage = "a" }, + new TestTurn { Index = 1, UserMessage = "b" } + ] + }, "run-1", model: null, CancellationToken.None); + + // Two turns of ~120ms each, summed onto the case and recorded per turn. + Assert.True(result.ModelDurationMs >= 200, $"was {result.ModelDurationMs}"); + Assert.True(result.Turns[0].ModelDurationMs >= 100, $"was {result.Turns[0].ModelDurationMs}"); + Assert.Equal( + result.ModelDurationMs, + result.Turns.Sum(t => t.ModelDurationMs)); + + // And it is never larger than the case's own wall clock, which contains it. + Assert.True(result.DurationMs >= result.ModelDurationMs); + } + + [Fact] + public async Task A_case_that_never_reached_the_model_reports_zero_model_time() + { + // What keeps a crashed run from looking like the fastest one on record: the executor drops + // these from the latency percentile, and it can only do that because the figure is zero + // rather than the wall clock of the failure. + var driver = new FakeDriver { CanaryResult = false }; + var runner = Build(driver, out _); + + var result = await runner.RunAsync(Suite(), new AgentTestCase + { + Id = "case-1", + Name = "c", + Turns = [new TestTurn { Index = 0, UserMessage = "a" }] + }, "run-1", model: null, CancellationToken.None); + + Assert.Equal(AgentTestStatus.Error, result.Status); + Assert.Equal(0, result.ModelDurationMs); + } } diff --git a/tests/BotSharp.Core.UnitTests/AgentTesting/AgentTestControllerTests.cs b/tests/BotSharp.Core.UnitTests/AgentTesting/AgentTestControllerTests.cs index bac90f1ab..165e21478 100644 --- a/tests/BotSharp.Core.UnitTests/AgentTesting/AgentTestControllerTests.cs +++ b/tests/BotSharp.Core.UnitTests/AgentTesting/AgentTestControllerTests.cs @@ -5,6 +5,7 @@ using System.Security.Claims; using System.Threading.Tasks; using BotSharp.Abstraction.Agents; +using BotSharp.Abstraction.Agents.Models; using BotSharp.Abstraction.Infrastructures.Attributes; using BotSharp.Abstraction.MLTasks; using BotSharp.Abstraction.MLTasks.Settings; @@ -110,8 +111,25 @@ private static ILlmProviderService ProviderServiceKnowing(params string[] provid return mock.Object; } + /// + /// An IAgentService that resolves exactly the given ids. The default Mock.Of<IAgentService> + /// resolves nothing, which is correct for every test that leaves EntryAgentId blank (the check + /// short-circuits there) but would reject any test that sets one. + /// + private static IAgentService AgentServiceKnowing(params string[] agentIds) + { + var known = new HashSet(agentIds, StringComparer.OrdinalIgnoreCase); + var mock = new Mock(); + mock.Setup(x => x.GetAgent(It.IsAny())) + .ReturnsAsync((string id) => known.Contains(id) ? new Agent { Id = id, Name = id } : null!); + return mock.Object; + } + private static AgentTestController BuildController( - InMemoryRepo repo, out RecordingQueue queue, ILlmProviderService? llmProviders = null) + InMemoryRepo repo, + out RecordingQueue queue, + ILlmProviderService? llmProviders = null, + IAgentService? agents = null) { var recorder = new AgentTestRecorder( Mock.Of(), @@ -120,7 +138,7 @@ private static AgentTestController BuildController( queue = new RecordingQueue(); var controller = new AgentTestController( - repo, queue, Mock.Of(), recorder, llmProviders ?? ProviderServiceKnowing()); + repo, queue, agents ?? Mock.Of(), recorder, llmProviders ?? ProviderServiceKnowing()); // TriggerRun reads User.FindFirstValue(ClaimTypes.NameIdentifier) -- a directly-constructed // controller (no MVC pipeline/TestServer) has no HttpContext at all by default, and @@ -610,4 +628,841 @@ public async Task CancelRun_still_accepts_a_run_that_has_not_finished(string liv Assert.IsType(result); Assert.True(repo.Runs["run-1"].CancelRequested); } + + private static AgentTestCaseUpsertRequest RoutingCaseRequest( + int turns = 1, params TestAssertion[] assertions) => new() + { + SuiteId = "suite-1", + Name = "routing case", + CaseType = CaseTypes.Routing, + Turns = Enumerable.Range(0, turns) + .Select(i => new TestTurn { Index = i, UserMessage = "hi" }) + .ToList(), + Assertions = assertions.ToList() + }; + + private static InMemoryRepo RepoWithSuite() + { + var repo = new InMemoryRepo(); + repo.Suites["suite-1"] = new AgentTestSuite { Id = "suite-1", AgentId = "agent-1", Name = "s" }; + return repo; + } + + [Fact] + public async Task An_unknown_case_type_is_rejected_rather_than_stored_as_the_default() + { + // Silently storing "Rounting" as Agent would leave the author with a case that reads + // routing-shaped in the UI and is never counted towards routing accuracy -- the failure mode + // is a gate figure quietly measuring fewer cases than anyone thinks. + var controller = BuildController(RepoWithSuite(), out _); + + var request = RoutingCaseRequest( + assertions: new TestAssertion { Type = AssertionTypes.RoutedToAgent, Expected = "WO" }); + request.CaseType = "Rounting"; + + var response = await controller.CreateCase(request); + + var bad = Assert.IsType(response.Result); + Assert.Contains("caseType", bad.Value?.ToString()); + } + + [Theory] + [InlineData("routing")] + [InlineData("ROUTING")] + public async Task A_case_type_is_stored_in_its_canonical_casing(string sent) + { + // Every comparison against CaseTypes.Routing is Ordinal, so storing the caller's casing would + // leave a case that never matches -- it would run, and then not be counted. + var repo = RepoWithSuite(); + var controller = BuildController(repo, out _); + + var request = RoutingCaseRequest( + assertions: new TestAssertion { Type = AssertionTypes.RoutedToAgent, Expected = "WO" }); + request.CaseType = sent; + + await controller.CreateCase(request); + + Assert.Equal(CaseTypes.Routing, Assert.Single(repo.Cases.Values).CaseType); + } + + [Fact] + public async Task A_case_that_omits_the_type_is_an_agent_case() + { + // Backward compatibility for every client written before the field existed, and for the + // documents already in the store: both have to keep meaning Agent. + var repo = RepoWithSuite(); + var controller = BuildController(repo, out _); + + await controller.CreateCase(new AgentTestCaseUpsertRequest + { + SuiteId = "suite-1", + Name = "c", + Turns = [new TestTurn { Index = 0, UserMessage = "hi" }] + }); + + Assert.Equal(CaseTypes.Agent, Assert.Single(repo.Cases.Values).CaseType); + } + + [Theory] + [InlineData(0)] + [InlineData(2)] + public async Task A_routing_case_must_have_exactly_one_turn(int turns) + { + // Routing is a single-turn question: which agent picks this message up. A second turn asks + // something else, and its verdict would still land in the routing accuracy figure. + var controller = BuildController(RepoWithSuite(), out _); + + var response = await controller.CreateCase(RoutingCaseRequest( + turns, new TestAssertion { Type = AssertionTypes.RoutedToAgent, Expected = "WO" })); + + var bad = Assert.IsType(response.Result); + Assert.Contains("exactly one turn", bad.Value?.ToString()); + } + + [Fact] + public async Task A_routing_case_without_a_routing_assertion_is_rejected() + { + // Otherwise the case counts towards routing accuracy while asserting nothing about routing: + // it reports Passed for having successfully said anything at all. + var controller = BuildController(RepoWithSuite(), out _); + + var response = await controller.CreateCase(RoutingCaseRequest( + assertions: new TestAssertion { Type = AssertionTypes.OutputContains, Expected = "hello" })); + + var bad = Assert.IsType(response.Result); + Assert.Contains(AssertionTypes.RoutedToAgent, bad.Value?.ToString()); + } + + [Fact] + public async Task Either_routing_assertion_type_satisfies_that_requirement() + { + // agentChain is the assertion that can actually describe a hand-off, so it has to count -- + // requiring routedToAgent specifically would force every routing case onto the weaker one. + var repo = RepoWithSuite(); + var controller = BuildController(repo, out _); + + var response = await controller.CreateCase(RoutingCaseRequest( + assertions: new TestAssertion + { + Type = AssertionTypes.AgentChain, + Target = AgentChainModes.Exact, + Expected = "Work Order Creator" + })); + + Assert.Null(response.Result); + Assert.Equal(CaseTypes.Routing, Assert.Single(repo.Cases.Values).CaseType); + } + + [Fact] + public async Task A_turn_level_routing_assertion_counts_too() + { + // A single-turn routing case can just as reasonably put its assertion on the turn as at case + // level; looking only at case-level assertions would reject a perfectly good case. + var repo = RepoWithSuite(); + var controller = BuildController(repo, out _); + + var response = await controller.CreateCase(new AgentTestCaseUpsertRequest + { + SuiteId = "suite-1", + Name = "c", + CaseType = CaseTypes.Routing, + Turns = + [ + new TestTurn + { + Index = 0, + UserMessage = "hi", + Assertions = [new TestAssertion { Type = AssertionTypes.RoutedToAgent, Expected = "WO" }] + } + ] + }); + + Assert.Null(response.Result); + Assert.Single(repo.Cases.Values); + } + + [Fact] + public async Task A_routing_case_cannot_use_the_llm_judge() + { + // Routing is scored purely as expected-agent == actual-agent. An llmJudge would also make the + // routing figure depend on a vendor call, so a vendor outage would read as a routing + // regression. + var controller = BuildController(RepoWithSuite(), out _); + + var response = await controller.CreateCase(RoutingCaseRequest( + 1, + new TestAssertion { Type = AssertionTypes.RoutedToAgent, Expected = "WO" }, + new TestAssertion { Type = AssertionTypes.LlmJudge, Expected = "is polite", MinScore = 4 })); + + var bad = Assert.IsType(response.Result); + Assert.Contains(AssertionTypes.LlmJudge, bad.Value?.ToString()); + } + + [Fact] + public async Task An_agent_case_is_not_held_to_the_routing_rules() + { + // Those rules are Routing-only. An Agent case is normally multi-turn and may well use + // llmJudge; applying routing's constraints to it would break every case already stored. + var repo = RepoWithSuite(); + var controller = BuildController(repo, out _); + + var response = await controller.CreateCase(new AgentTestCaseUpsertRequest + { + SuiteId = "suite-1", + Name = "c", + CaseType = CaseTypes.Agent, + Turns = + [ + new TestTurn { Index = 0, UserMessage = "a" }, + new TestTurn { Index = 1, UserMessage = "b" } + ], + Assertions = [new TestAssertion { Type = AssertionTypes.LlmJudge, Expected = "is polite", MinScore = 4 }] + }); + + Assert.Null(response.Result); + Assert.Single(repo.Cases.Values); + } + + [Fact] + public async Task An_entry_agent_that_does_not_exist_is_rejected_at_save_time() + { + // A typo here would otherwise turn every run of the case into an opaque infrastructure Error: + // the canary fails against an agent BotSharp cannot load, and its message says nothing about + // the real cause. + var controller = BuildController(RepoWithSuite(), out _, agents: AgentServiceKnowing("copilot-entry")); + + var request = RoutingCaseRequest( + assertions: new TestAssertion { Type = AssertionTypes.RoutedToAgent, Expected = "WO" }); + request.EntryAgentId = "coplot-entry"; + + var response = await controller.CreateCase(request); + + var bad = Assert.IsType(response.Result); + Assert.Contains("coplot-entry", bad.Value?.ToString()); + } + + [Fact] + public async Task A_known_entry_agent_is_stored_trimmed() + { + var repo = RepoWithSuite(); + var controller = BuildController(repo, out _, agents: AgentServiceKnowing("copilot-entry")); + + var request = RoutingCaseRequest( + assertions: new TestAssertion { Type = AssertionTypes.RoutedToAgent, Expected = "WO" }); + request.EntryAgentId = " copilot-entry "; + + await controller.CreateCase(request); + + Assert.Equal("copilot-entry", Assert.Single(repo.Cases.Values).EntryAgentId); + } + + [Fact] + public async Task A_blank_entry_agent_is_stored_as_null_and_needs_no_lookup() + { + // Null is what the runner's "fall back to the suite's agent" check reads. A UI posting "" for + // an untouched field must not retarget the case at an agent id of "", and saving a case must + // not require knowing any agent id at all -- note this controller resolves no agents. + var repo = RepoWithSuite(); + var controller = BuildController(repo, out _); + + var request = RoutingCaseRequest( + assertions: new TestAssertion { Type = AssertionTypes.RoutedToAgent, Expected = "WO" }); + request.EntryAgentId = " "; + + await controller.CreateCase(request); + + Assert.Null(Assert.Single(repo.Cases.Values).EntryAgentId); + } + + [Fact] + public async Task E2E_is_no_longer_an_accepted_case_type() + { + // Dropped by project owner decision: a multi-agent journey is an Agent case whose agentChain + // assertion describes the hand-offs, so a third type bought nothing but a third branch in + // every validation and aggregation path. + var controller = BuildController(RepoWithSuite(), out _); + + var request = RoutingCaseRequest( + assertions: new TestAssertion { Type = AssertionTypes.RoutedToAgent, Expected = "WO" }); + request.CaseType = "E2E"; + + var response = await controller.CreateCase(request); + + var bad = Assert.IsType(response.Result); + Assert.Contains("caseType", bad.Value?.ToString()); + } + + [Theory] + [InlineData("system")] + [InlineData("function")] + [InlineData("tool")] + [InlineData("")] + public async Task An_unsupported_history_role_is_rejected(string role) + { + // system would compete with the agent's own instruction, and function would fake a tool call + // -- letting a case claim a tool ran when nothing did. Both are worse than a save error. + var controller = BuildController(RepoWithSuite(), out _); + + var response = await controller.CreateCase(new AgentTestCaseUpsertRequest + { + SuiteId = "suite-1", + Name = "c", + History = [new TestHistoryMessage { Role = role, Content = "hello" }], + Turns = [new TestTurn { Index = 0, UserMessage = "hi" }] + }); + + var bad = Assert.IsType(response.Result); + Assert.Contains("history message 1", bad.Value?.ToString()); + } + + [Fact] + public async Task A_history_message_with_no_content_is_rejected() + { + // BotSharp's own dialog storage drops elements with blank content, so it would not be there + // at run time, and the runner's write-count check would then fail the case with a confusing + // message about the write having vanished. + var controller = BuildController(RepoWithSuite(), out _); + + var response = await controller.CreateCase(new AgentTestCaseUpsertRequest + { + SuiteId = "suite-1", + Name = "c", + History = [new TestHistoryMessage { Role = HistoryRoles.User, Content = " " }], + Turns = [new TestTurn { Index = 0, UserMessage = "hi" }] + }); + + var bad = Assert.IsType(response.Result); + Assert.Contains("no content", bad.Value?.ToString()); + } + + [Fact] + public async Task A_history_role_is_stored_in_its_canonical_casing() + { + // The driver and every later comparison use the lowercase constants. + var repo = RepoWithSuite(); + var controller = BuildController(repo, out _); + + await controller.CreateCase(new AgentTestCaseUpsertRequest + { + SuiteId = "suite-1", + Name = "c", + History = [new TestHistoryMessage { Role = "ASSISTANT", Content = "hello" }], + Turns = [new TestTurn { Index = 0, UserMessage = "hi" }] + }); + + var stored = Assert.Single(repo.Cases.Values); + Assert.Equal(HistoryRoles.Assistant, Assert.Single(stored.History).Role); + } + + [Fact] + public async Task History_does_not_count_towards_a_routing_cases_one_turn_limit() + { + // Replaying a prior exchange and then asking one question is still a single routing decision, + // and it is the most realistic way to test routing that depends on context. Counting history + // as turns would make that impossible to express. + var repo = RepoWithSuite(); + var controller = BuildController(repo, out _); + + var request = RoutingCaseRequest( + assertions: new TestAssertion { Type = AssertionTypes.RoutedToAgent, Expected = "WO" }); + request.History = + [ + new TestHistoryMessage { Role = HistoryRoles.User, Content = "my fridge is leaking" }, + new TestHistoryMessage { Role = HistoryRoles.Assistant, Content = "I raised work order B123." } + ]; + + var response = await controller.CreateCase(request); + + Assert.Null(response.Result); + Assert.Equal(2, Assert.Single(repo.Cases.Values).History.Count); + } + + /// + /// A case with every field populated, so a copy test fails when a field is dropped rather than + /// only when the obvious ones are. + /// + private static AgentTestCase FullyPopulatedCase() => new() + { + Id = "case-1", + SuiteId = "suite-1", + Name = "asking for an ETA", + Enabled = true, + CaseType = CaseTypes.Routing, + EntryAgentId = "copilot-entry", + History = [new TestHistoryMessage { Role = HistoryRoles.User, Content = "my fridge is leaking" }], + Turns = + [ + new TestTurn + { + Index = 0, + UserMessage = "when is someone coming?", + Assertions = [new TestAssertion { Type = AssertionTypes.RoutedToAgent, Expected = "WO", Fatal = true }] + } + ], + Assertions = + [ + new TestAssertion + { + Type = AssertionTypes.AgentChain, + Target = AgentChainModes.Ordered, + Expected = "Copilot, WO" + } + ], + InitialStates = [new TestState { Key = "wo_num", Value = "B123", ActiveRounds = 3, Global = true }], + Mocks = + [ + new TestToolMock + { + FunctionName = "get_estimate_arrival_time", + ArgsMatchJson = "{\"wo_num\":\"B123\"}", + CallIndex = 1, + ResultContent = "tomorrow 9am", + StopCompletion = true, + StateWrites = [new TestState { Key = "eta", Value = "tomorrow" }] + } + ], + UnmockedToolPolicy = UnmockedToolPolicies.Block, + SourceConversationId = "conv-9" + }; + + [Fact] + public async Task Copying_a_case_carries_every_field() + { + // The reason this endpoint exists server-side at all. A client that rebuilds the payload from + // its own form drops whatever it does not know about, and a copy missing its mocks looks + // identical in the list right up to the run where it blocks every tool. + var repo = RepoWithSuite(); + var source = FullyPopulatedCase(); + repo.Cases[source.Id] = source; + var controller = BuildController(repo, out _); + + var response = await controller.CopyCase("case-1"); + + var copy = Assert.IsType(response.Value); + Assert.Equal(source.SuiteId, copy.SuiteId); + Assert.Equal(source.CaseType, copy.CaseType); + Assert.Equal(source.EntryAgentId, copy.EntryAgentId); + Assert.Equal(source.UnmockedToolPolicy, copy.UnmockedToolPolicy); + Assert.Equal(source.SourceConversationId, copy.SourceConversationId); + + Assert.Equal("my fridge is leaking", Assert.Single(copy.History).Content); + Assert.Equal("when is someone coming?", Assert.Single(copy.Turns).UserMessage); + Assert.True(Assert.Single(Assert.Single(copy.Turns).Assertions).Fatal); + Assert.Equal(AgentChainModes.Ordered, Assert.Single(copy.Assertions).Target); + + var state = Assert.Single(copy.InitialStates); + Assert.Equal("wo_num", state.Key); + Assert.Equal(3, state.ActiveRounds); + Assert.True(state.Global); + + var mock = Assert.Single(copy.Mocks); + Assert.Equal("get_estimate_arrival_time", mock.FunctionName); + Assert.Equal("{\"wo_num\":\"B123\"}", mock.ArgsMatchJson); + Assert.Equal(1, mock.CallIndex); + Assert.True(mock.StopCompletion); + Assert.Equal("eta", Assert.Single(mock.StateWrites!).Key); + } + + [Fact] + public async Task A_copy_lands_disabled_even_when_the_source_was_enabled() + { + // An exact duplicate joining the next run measures the same thing twice: it pads the + // pass-rate denominator, and for a routing case it double-weights one routing decision. The + // copy waits for the edit it was made for. + var repo = RepoWithSuite(); + repo.Cases["case-1"] = FullyPopulatedCase(); + var controller = BuildController(repo, out _); + + var response = await controller.CopyCase("case-1"); + + Assert.False(Assert.IsType(response.Value).Enabled); + // And the source is untouched. + Assert.True(repo.Cases["case-1"].Enabled); + } + + [Fact] + public async Task A_copy_gets_its_own_id_and_does_not_overwrite_the_source() + { + // The BSON round trip copies the source's _id, so failing to blank it would turn the copy + // into a full overwrite of the original -- the worst possible outcome for a copy button. + var repo = RepoWithSuite(); + repo.Cases["case-1"] = FullyPopulatedCase(); + var controller = BuildController(repo, out _); + + var response = await controller.CopyCase("case-1"); + + var copy = Assert.IsType(response.Value); + Assert.NotEqual("case-1", copy.Id); + Assert.NotEmpty(copy.Id); + Assert.Equal(2, repo.Cases.Count); + Assert.Equal("asking for an ETA", repo.Cases["case-1"].Name); + } + + [Fact] + public async Task Editing_a_copy_does_not_reach_back_into_the_source() + { + // A shallow clone would have both documents sharing the same Turns/Mocks list instances, so + // the first edit to the copy would silently rewrite the case it came from. + var repo = RepoWithSuite(); + repo.Cases["case-1"] = FullyPopulatedCase(); + var controller = BuildController(repo, out _); + + var copy = Assert.IsType((await controller.CopyCase("case-1")).Value); + + copy.Turns[0].UserMessage = "changed"; + copy.Mocks[0].ResultContent = "changed"; + copy.History[0].Content = "changed"; + + var source = repo.Cases["case-1"]; + Assert.Equal("when is someone coming?", source.Turns[0].UserMessage); + Assert.Equal("tomorrow 9am", source.Mocks[0].ResultContent); + Assert.Equal("my fridge is leaking", source.History[0].Content); + } + + [Fact] + public async Task Repeated_copies_get_distinguishable_names() + { + // Two rows both called "x (copy)" cannot be told apart in the list, which is the one place + // copies are managed. + var repo = RepoWithSuite(); + repo.Cases["case-1"] = FullyPopulatedCase(); + var controller = BuildController(repo, out _); + + var first = Assert.IsType((await controller.CopyCase("case-1")).Value); + var second = Assert.IsType((await controller.CopyCase("case-1")).Value); + var third = Assert.IsType((await controller.CopyCase("case-1")).Value); + + Assert.Equal("asking for an ETA (copy)", first.Name); + Assert.Equal("asking for an ETA (copy 2)", second.Name); + Assert.Equal("asking for an ETA (copy 3)", third.Name); + } + + [Fact] + public async Task A_copy_of_a_copy_is_named_from_the_case_it_was_copied_from() + { + // Not "x (copy) (copy)": the suffix is appended to whatever the source is called, and the + // collision check is what keeps the result unique. + var repo = RepoWithSuite(); + repo.Cases["case-1"] = FullyPopulatedCase(); + var controller = BuildController(repo, out _); + + var first = Assert.IsType((await controller.CopyCase("case-1")).Value); + var nested = Assert.IsType((await controller.CopyCase(first.Id)).Value); + + Assert.Equal("asking for an ETA (copy) (copy)", nested.Name); + } + + [Fact] + public async Task A_copied_name_stays_short_enough_to_edit() + { + // A name the case editor's own input cannot hold would have to be trimmed by hand before any + // other change to the copy could be saved. + var repo = RepoWithSuite(); + var source = FullyPopulatedCase(); + source.Name = new string('x', 200); + repo.Cases["case-1"] = source; + var controller = BuildController(repo, out _); + + var copy = Assert.IsType((await controller.CopyCase("case-1")).Value); + + Assert.True(copy.Name.Length <= 200, $"name was {copy.Name.Length} characters"); + Assert.EndsWith(" (copy)", copy.Name); + } + + [Fact] + public async Task A_copy_does_not_inherit_the_sources_create_date() + { + // The round trip copies it, which would have the copy claim to be as old as the case it came + // from -- and the case list is sorted newest first, so a fresh copy would appear buried. + var repo = RepoWithSuite(); + var source = FullyPopulatedCase(); + source.CreateDate = new DateTime(2020, 1, 1, 0, 0, 0, DateTimeKind.Utc); + repo.Cases["case-1"] = source; + var controller = BuildController(repo, out _); + + var copy = Assert.IsType((await controller.CopyCase("case-1")).Value); + + Assert.True(copy.CreateDate > new DateTime(2021, 1, 1, 0, 0, 0, DateTimeKind.Utc)); + } + + [Fact] + public async Task Copying_a_case_that_does_not_exist_is_a_404() + { + var controller = BuildController(RepoWithSuite(), out _); + + var response = await controller.CopyCase("nope"); + + Assert.IsType(response.Result); + } + + // ------------------------------------------------------------- governance metadata + + [Theory] + [InlineData("P3")] + [InlineData("high")] + public async Task An_unknown_priority_is_rejected(string priority) + { + // Priority decides the batch, and a batch decides whether a failure stops the evaluation. + // Storing "high" would leave a case that matches no priority and lands in the default batch + // while its author believes it is stop-loss. + var repo = RepoWithSuite(); + var controller = BuildController(repo, out _); + + var response = await controller.CreateCase(new AgentTestCaseUpsertRequest + { + SuiteId = "suite-1", + Name = "c", + Priority = priority, + Turns = [new TestTurn { Index = 0, UserMessage = "hi" }] + }); + + var bad = Assert.IsType(response.Result); + Assert.Contains("priority", bad.Value?.ToString()); + } + + [Fact] + public async Task An_unknown_severity_is_rejected() + { + var controller = BuildController(RepoWithSuite(), out _); + + var response = await controller.CreateCase(new AgentTestCaseUpsertRequest + { + SuiteId = "suite-1", + Name = "c", + Severity = "critical", + Turns = [new TestTurn { Index = 0, UserMessage = "hi" }] + }); + + var bad = Assert.IsType(response.Result); + Assert.Contains("severity", bad.Value?.ToString()); + } + + [Fact] + public async Task An_out_of_range_batch_is_rejected_rather_than_clamped() + { + // Clamping 4 to 3 would file the case somewhere its author never asked for, silently, in the + // batch that does not block a release. + var controller = BuildController(RepoWithSuite(), out _); + + var response = await controller.CreateCase(new AgentTestCaseUpsertRequest + { + SuiteId = "suite-1", + Name = "c", + Batch = 4, + Turns = [new TestTurn { Index = 0, UserMessage = "hi" }] + }); + + var bad = Assert.IsType(response.Result); + Assert.Contains("batch", bad.Value?.ToString()); + } + + [Fact] + public async Task Priority_and_severity_are_stored_in_canonical_casing() + { + // Every comparison against CasePriorities.P0 is Ordinal, so storing "p0" would leave a case + // that runs and is then filed in the wrong batch. + var repo = RepoWithSuite(); + var controller = BuildController(repo, out _); + + await controller.CreateCase(new AgentTestCaseUpsertRequest + { + SuiteId = "suite-1", + Name = "c", + Priority = "p0", + Severity = "s0", + Turns = [new TestTurn { Index = 0, UserMessage = "hi" }] + }); + + var stored = Assert.Single(repo.Cases.Values); + Assert.Equal(CasePriorities.P0, stored.Priority); + Assert.Equal(CaseSeverities.S0, stored.Severity); + } + + [Fact] + public async Task A_case_that_omits_the_governance_fields_gets_the_untriaged_defaults() + { + // P1 and S1 for every case stored before these fields existed: P0/S0 would make each of them + // an immediate no-go, and P2/S2 would drop them out of the mandatory batches and let a real + // failure read as an experience nit. + var repo = RepoWithSuite(); + var controller = BuildController(repo, out _); + + await controller.CreateCase(new AgentTestCaseUpsertRequest + { + SuiteId = "suite-1", + Name = "c", + Turns = [new TestTurn { Index = 0, UserMessage = "hi" }] + }); + + var stored = Assert.Single(repo.Cases.Values); + Assert.Equal(CasePriorities.P1, stored.Priority); + Assert.Equal(CaseSeverities.S1, stored.Severity); + Assert.Null(stored.Batch); + Assert.False(stored.CrossCutting); + Assert.Empty(stored.InvolvedAgents); + Assert.Null(stored.LastReviewedDate); + } + + [Fact] + public async Task Involved_agents_are_trimmed_and_deduplicated() + { + var repo = RepoWithSuite(); + var controller = BuildController(repo, out _); + + await controller.CreateCase(new AgentTestCaseUpsertRequest + { + SuiteId = "suite-1", + Name = "c", + InvolvedAgents = [" agent-a ", "AGENT-A", "", "agent-b"], + Turns = [new TestTurn { Index = 0, UserMessage = "hi" }] + }); + + Assert.Equal(["agent-a", "agent-b"], Assert.Single(repo.Cases.Values).InvolvedAgents); + } + + [Fact] + public async Task Saving_a_case_never_stamps_the_reviewed_date() + { + // A case can be edited many times and still rest on an assumption nobody has questioned in a + // year. Stamping this on every write would hide exactly that. + var repo = RepoWithSuite(); + var controller = BuildController(repo, out _); + + await controller.CreateCase(new AgentTestCaseUpsertRequest + { + SuiteId = "suite-1", + Name = "c", + Turns = [new TestTurn { Index = 0, UserMessage = "hi" }] + }); + + Assert.Null(Assert.Single(repo.Cases.Values).LastReviewedDate); + } + + // ------------------------------------------------------------- scope selection + + private static InMemoryRepo RepoWithScopedCases() + { + var repo = new InMemoryRepo(); + repo.Suites["suite-1"] = new AgentTestSuite { Id = "suite-1", AgentId = "agent-a", Name = "A suite" }; + repo.Cases["on-target"] = new AgentTestCase + { + Id = "on-target", SuiteId = "suite-1", Name = "on target", EntryAgentId = "agent-a" + }; + repo.Cases["off-target"] = new AgentTestCase + { + Id = "off-target", SuiteId = "suite-1", Name = "off target", EntryAgentId = "agent-b" + }; + repo.Cases["safety"] = new AgentTestCase + { + Id = "safety", SuiteId = "suite-1", Name = "safety", EntryAgentId = "agent-b", CrossCutting = true + }; + repo.Cases["draft"] = new AgentTestCase + { + Id = "draft", SuiteId = "suite-1", Name = "draft", EntryAgentId = "agent-a", Enabled = false + }; + return repo; + } + + [Fact] + public async Task A_scope_with_no_targets_and_no_platform_flag_is_rejected() + { + // It would narrow to nothing, and an empty scope reported as a successful plan is the single + // most dangerous answer this endpoint could give: it reads as "nothing needs testing". + var controller = BuildController(RepoWithScopedCases(), out _); + + var response = await controller.SelectScope(new ScopeSelectionRequest()); + + Assert.IsType(response.Result); + } + + [Fact] + public async Task A_scope_reports_both_halves_with_a_reason_for_each() + { + // The excluded half is the one worth reading: an excluded case produces no result to notice, + // so the only defence is being able to see what was left out and why. + var controller = BuildController(RepoWithScopedCases(), out _); + + var response = await controller.SelectScope(new ScopeSelectionRequest + { + TargetAgentIds = ["agent-a"] + }); + + var scope = Assert.IsType(response.Value); + Assert.Equal(4, scope.TotalCases); + + Assert.Equal( + ["on-target", "safety"], + scope.Included.Select(c => c.CaseId).OrderBy(id => id).ToList()); + Assert.Equal(ScopeReasons.TargetAgent, scope.Included.Single(c => c.CaseId == "on-target").Reason); + Assert.Equal(ScopeReasons.CrossCutting, scope.Included.Single(c => c.CaseId == "safety").Reason); + + Assert.Equal(ScopeReasons.NotInvolved, scope.Excluded.Single(c => c.CaseId == "off-target").Reason); + Assert.Equal(ScopeReasons.Disabled, scope.Excluded.Single(c => c.CaseId == "draft").Reason); + } + + [Fact] + public async Task A_platform_wide_scope_includes_every_enabled_case() + { + // Narrowing switches off: there is no agent a foundation model swap demonstrably does not + // touch. Disabled cases stay out, because no run would execute them. + var controller = BuildController(RepoWithScopedCases(), out _); + + var response = await controller.SelectScope(new ScopeSelectionRequest { FullPlatform = true }); + + var scope = Assert.IsType(response.Value); + Assert.Equal(3, scope.Included.Count); + Assert.Equal("draft", Assert.Single(scope.Excluded).CaseId); + } + + [Fact] + public async Task A_scope_carries_the_metadata_the_decision_was_made_from() + { + // A verdict on its own cannot be reviewed. The involved set and the effective batch are what + // let someone check the plan rather than trust it. + var controller = BuildController(RepoWithScopedCases(), out _); + + var response = await controller.SelectScope(new ScopeSelectionRequest + { + TargetAgentIds = ["agent-a"] + }); + + var scope = Assert.IsType(response.Value); + var onTarget = scope.Included.Single(c => c.CaseId == "on-target"); + + Assert.Equal(["agent-a"], onTarget.InvolvedAgentIds); + Assert.Equal(CaseBatches.Mandatory, onTarget.Batch); + Assert.Equal("A suite", onTarget.SuiteName); + // Cross-cutting forces batch 1, and the response has to agree with that. + Assert.Equal(CaseBatches.StopLoss, scope.Included.Single(c => c.CaseId == "safety").Batch); + } + + [Fact] + public async Task A_scope_can_be_narrowed_to_one_batch() + { + var controller = BuildController(RepoWithScopedCases(), out _); + + var response = await controller.SelectScope(new ScopeSelectionRequest + { + TargetAgentIds = ["agent-a"], + Batch = CaseBatches.StopLoss + }); + + var scope = Assert.IsType(response.Value); + Assert.Equal("safety", Assert.Single(scope.Included).CaseId); + Assert.Equal(ScopeReasons.OtherBatch, scope.Excluded.Single(c => c.CaseId == "on-target").Reason); + } + + [Fact] + public async Task An_out_of_range_batch_on_a_scope_is_rejected() + { + var controller = BuildController(RepoWithScopedCases(), out _); + + var response = await controller.SelectScope(new ScopeSelectionRequest + { + TargetAgentIds = ["agent-a"], + Batch = 7 + }); + + Assert.IsType(response.Result); + } } diff --git a/tests/BotSharp.Core.UnitTests/AgentTesting/AgentTestJudgeTests.cs b/tests/BotSharp.Core.UnitTests/AgentTesting/AgentTestJudgeTests.cs new file mode 100644 index 000000000..84592a82e --- /dev/null +++ b/tests/BotSharp.Core.UnitTests/AgentTesting/AgentTestJudgeTests.cs @@ -0,0 +1,231 @@ +using System; +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging.Abstractions; +using BotSharp.Plugin.AgentTesting.Models; +using BotSharp.Plugin.AgentTesting.Runtime; +using BotSharp.Plugin.AgentTesting.Services; +using Xunit; + +namespace BotSharp.Core.UnitTests.AgentTesting; + +/// +/// llmJudge is the one assertion type that is not a pure function, and that makes two things worth +/// pinning here. +/// +/// First, the model's answer is extended no trust: a blank reply, a non-JSON reply, malformed JSON or +/// a score off the 1-5 scale are all "no verdict", never a coerced pass or fail. A judge that ignored +/// the rubric has graded nothing, and reading pass/fail out of that is reading meaning into noise. +/// +/// Second, and more important, every way the judge can fail to reach a verdict has to land on Error, +/// not Failed. A vendor timeout, an unconfigured judge model or a rate limit says nothing about the +/// agent under test -- if any of them surfaced as a failing assertion, provider noise would be +/// indistinguishable from an agent regression, which is the whole reason the harness keeps Failed and +/// Error apart. +/// +public class AgentTestJudgeTests +{ + // ---- ParseVerdict: extend the model's answer no trust --------------------------------------- + + [Fact] + public void Parses_a_plain_json_verdict() + { + var verdict = LlmAgentTestJudge.ParseVerdict("""{"score": 4, "reason": "asks for the work order number"}"""); + + Assert.Equal(4, verdict.Score); + Assert.Equal("asks for the work order number", verdict.Reason); + } + + [Fact] + public void Parses_a_verdict_wrapped_in_a_code_fence_or_prose() + { + // Models add fences and preambles despite being told not to. Rejecting those would make the + // feature fail for a formatting habit rather than for a real problem. + var verdict = LlmAgentTestJudge.ParseVerdict( + "Sure, here is my assessment:\n```json\n{\"score\": 5, \"reason\": \"clear\"}\n```\n"); + + Assert.Equal(5, verdict.Score); + Assert.Equal("clear", verdict.Reason); + } + + [Fact] + public void Accepts_property_names_in_any_case() + { + var verdict = LlmAgentTestJudge.ParseVerdict("""{"Score": 3, "Reason": "partial"}"""); + + Assert.Equal(3, verdict.Score); + Assert.Equal("partial", verdict.Reason); + } + + [Fact] + public void Accepts_a_verdict_with_no_reason() + { + // The reason is for a human reading the result afterwards; its absence must not invalidate a + // score the model did give. + var verdict = LlmAgentTestJudge.ParseVerdict("""{"score": 5}"""); + + Assert.Equal(5, verdict.Score); + Assert.Null(verdict.Reason); + } + + [Theory] + [InlineData("")] + [InlineData("I would rate this a 4 out of 5.")] // no JSON at all + [InlineData("{\"score\": }")] // malformed + [InlineData("{\"reason\": \"good\"}")] // no score -> defaults to 0, off scale + public void Rejects_a_reply_it_cannot_read_as_a_score(string raw) + { + Assert.Throws(() => LlmAgentTestJudge.ParseVerdict(raw)); + } + + [Theory] + [InlineData(0)] + [InlineData(6)] + [InlineData(-1)] + [InlineData(100)] + public void Rejects_a_score_outside_the_scale_instead_of_clamping_it(double score) + { + // Clamping would silently turn "did not follow the rubric" into a grade. 6 clamped to 5 + // would even pass, which is the worst of the available outcomes. + var ex = Assert.Throws( + () => LlmAgentTestJudge.ParseVerdict($"{{\"score\": {score}}}")); + + Assert.Contains("1-5", ex.Message); + } + + // ---- JudgeAsync guards: everything decidable without a vendor ------------------------------- + + [Fact] + public async Task Refuses_to_judge_when_the_suite_has_no_judge_model() + { + // The reason this check exists at all: BotSharp's InstructService silently falls back to + // openai/gpt-4o when no provider and model are given. Inheriting that would score cases with + // a model nobody chose, and the run would still look conclusive. + var ex = await Assert.ThrowsAsync( + () => Judge().JudgeAsync(Assertion(), Context("we need your work order number"), new AgentTestSuite + { + Id = "suite-1", + AgentId = "agent-1", + Name = "s" + }, CancellationToken.None)); + + // The message has to name the fix -- whoever sees this in the UI is the person who has to + // configure the suite. + Assert.Contains("judgeProvider", ex.Message); + Assert.Contains("judgeModel", ex.Message); + } + + [Fact] + public async Task Refuses_to_judge_an_assertion_with_no_criterion() + { + // AssertionValidation already rejects this at save time. Repeated in the judge because a case + // stored before that rule existed would otherwise reach the vendor with an empty criterion + // and come back with a meaningless score. + await Assert.ThrowsAsync( + () => Judge().JudgeAsync( + new TestAssertion { Type = AssertionTypes.LlmJudge, Expected = " " }, + Context("anything"), + ConfiguredSuite(), + CancellationToken.None)); + } + + [Fact] + public async Task Refuses_to_judge_when_the_agent_produced_no_reply() + { + // Not a failing verdict: there is nothing to grade. Whatever went wrong upstream is the real + // finding, and this case's other assertions report it far more usefully than a fabricated + // score would. + await Assert.ThrowsAsync( + () => Judge().JudgeAsync(Assertion(), Context(null), ConfiguredSuite(), CancellationToken.None)); + } + + [Fact] + public async Task Reports_an_unregistered_judge_provider_as_no_verdict() + { + var ex = await Assert.ThrowsAsync( + () => Judge().JudgeAsync(Assertion(), Context("some reply"), ConfiguredSuite(provider: "not-installed"), + CancellationToken.None)); + + Assert.Contains("not-installed", ex.Message); + } + + // ---- The Failed/Error split, end to end through the runner ---------------------------------- + + [Fact] + public async Task An_unjudgeable_case_is_Error_not_Failed() + { + // The single most important behaviour in this file. If this ever reports Failed, every + // vendor hiccup starts reading as an agent regression, and a run's Failed count stops + // meaning anything. + var driver = new StubDriver("we need your work order number"); + var runner = new AgentTestCaseRunner( + new AgentTestRunRegistry(), + driver, + NullLogger.Instance, + judge: null); // no judge registered at all + + var result = await runner.RunAsync(ConfiguredSuite(), new AgentTestCase + { + Id = "case-1", + SuiteId = "suite-1", + Name = "c", + Turns = [new TestTurn { Index = 0, UserMessage = "where is my work order" }], + Assertions = [Assertion()] + }, "run-1", null, CancellationToken.None); + + Assert.Equal(AgentTestStatus.Error, result.Status); + Assert.Contains("llmJudge", result.Error); + } + + // ---- helpers ------------------------------------------------------------------------------- + + private static LlmAgentTestJudge Judge() + => new(new ServiceCollection().BuildServiceProvider(), NullLogger.Instance); + + private static TestAssertion Assertion() => new() + { + Type = AssertionTypes.LlmJudge, + Expected = "the reply asks the user for a work order number" + }; + + private static AssertionContext Context(string? output) => new() { Output = output }; + + private static AgentTestSuite ConfiguredSuite(string provider = "openai") => new() + { + Id = "suite-1", + AgentId = "agent-1", + Name = "s", + CaseTimeoutSeconds = 120, + JudgeProvider = provider, + JudgeModel = "gpt-4o" + }; + + /// Minimal driver: a live seam and one canned reply, so the runner reaches the judge. + private sealed class StubDriver : IAgentConversationDriver + { + private readonly string _reply; + + public StubDriver(string reply) => _reply = reply; + + public Task PrepareAsync(string conversationId, string agentId, IReadOnlyList initialStates) + => Task.CompletedTask; + + public Task SendAsync(string conversationId, string agentId, string userMessage, CancellationToken ct) + => Task.FromResult(_reply); + + public Task RunCanaryAsync(string conversationId, string agentId, CancellationToken ct) + => Task.FromResult(true); + + public Task> ReadStatesAsync(string conversationId) + => Task.FromResult>(new Dictionary()); + + public Task> ReadAssistantAgentSequenceAsync(string conversationId) + => Task.FromResult>([]); + + public Task InjectHistoryAsync( + string conversationId, string agentId, IReadOnlyList history) + => Task.FromResult(history.Count); + } +} diff --git a/tests/BotSharp.Core.UnitTests/AgentTesting/AgentTestRunExecutorTests.cs b/tests/BotSharp.Core.UnitTests/AgentTesting/AgentTestRunExecutorTests.cs index eee4129f6..be3cf7434 100644 --- a/tests/BotSharp.Core.UnitTests/AgentTesting/AgentTestRunExecutorTests.cs +++ b/tests/BotSharp.Core.UnitTests/AgentTesting/AgentTestRunExecutorTests.cs @@ -8,6 +8,9 @@ using BotSharp.Plugin.AgentTesting.Services; using BotSharp.Plugin.AgentTesting.Models; using Xunit; +using BotSharp.Abstraction.MLTasks; +using BotSharp.Abstraction.MLTasks.Settings; +using Moq; namespace BotSharp.Core.UnitTests.AgentTesting; @@ -113,10 +116,30 @@ private static AgentTestRunExecutor Build(InMemoryRepo repo, Func BuildWithRunner(repo, run).Executor; private static (AgentTestRunExecutor Executor, DelegatingCaseRunner Runner) BuildWithRunner( - InMemoryRepo repo, Func run) + InMemoryRepo repo, + Func run, + ILlmProviderService? llmProviders = null) { var runner = new DelegatingCaseRunner(run); - return (new AgentTestRunExecutor(repo, runner, NullLogger.Instance), runner); + return ( + new AgentTestRunExecutor(repo, runner, NullLogger.Instance, llmProviders), + runner); + } + + /// An ILlmProviderService that knows one model's text token unit costs. + private static ILlmProviderService PricingFor(string provider, string model, float input, float output) + { + var mock = new Mock(); + mock.Setup(x => x.GetSetting(It.IsAny(), It.IsAny())) + .Returns((string p, string m) => + string.Equals(p, provider) && string.Equals(m, model) + ? new LlmModelSetting + { + Name = m, + Cost = new LlmCostSetting { TextInputCost = input, TextOutputCost = output } + } + : null); + return mock.Object; } private sealed class DelegatingCaseRunner(Func run) : ICaseRunner @@ -124,15 +147,25 @@ private sealed class DelegatingCaseRunner(FuncEvery (case, model) pair the executor asked for, in the order it asked. public List<(string CaseId, string? Model)> Invocations { get; } = []; + /// + /// Set instead of relying on the constructor delegate when a test's outcome has to depend on + /// WHICH model ran the case -- the only way to exercise a per-model figure such as routing + /// accuracy without coupling the test to the executor's (case, model) iteration order. + /// + public Func? RunWithModel { get; set; } + public Task RunAsync( AgentTestSuite suite, AgentTestCase testCase, string runId, TestModel? model, CancellationToken ct) { Invocations.Add((testCase.Id, model?.Model)); - var result = run(testCase); + var result = RunWithModel != null ? RunWithModel(testCase, model) : run(testCase); // The real runner stamps these onto the result; mirror it so tests can assert that a // result can be attributed back to the model that produced it. result.Provider ??= model?.Provider; result.Model ??= model?.Model; + // Same reason: the real runner copies the case type onto the result, and the executor's + // routing tally reads it back off the result rather than off the case. + result.CaseType = testCase.CaseType; return Task.FromResult(result); } } @@ -411,4 +444,244 @@ public async Task A_cancel_that_arrives_while_the_only_case_is_executing_still_s // happened. Assert.True(repo.Stored.CancelRequested); } + + private static AgentTestCase RoutingCase(string id) => new() + { + Id = id, SuiteId = "suite-1", Name = id, + CaseType = CaseTypes.Routing, + Turns = [new TestTurn { Index = 0, UserMessage = "hi" }] + }; + + [Fact] + public async Task Routing_accuracy_counts_only_routing_cases() + { + // The evaluation framework gates routing accuracy separately from the agent pass rate, so + // folding an agent case into this figure makes the gate unreadable -- and it is the easiest + // mistake to make, since every case result flows through the same tally. + var repo = new InMemoryRepo { Cases = [RoutingCase("r1"), RoutingCase("r2"), CaseNamed("a1")] }; + var executor = Build(repo, c => new AgentTestCaseResult + { + CaseId = c.Id, + Status = c.Id == "r2" ? AgentTestStatus.Failed : AgentTestStatus.Passed + }); + + await executor.ExecuteAsync("run-1", CancellationToken.None); + + var accuracy = Assert.Single(repo.Run.RoutingAccuracies); + Assert.Equal(2, accuracy.CaseCount); + Assert.Equal(1, accuracy.PassedCount); + + // The agent case still counts in the run's own totals; it is only absent from the routing + // figure. + Assert.Equal(3, repo.Run.TotalCount); + Assert.Equal(2, repo.Run.PassedCount); + } + + [Fact] + public async Task An_errored_routing_case_counts_against_accuracy_rather_than_being_skipped() + { + // "Could not tell" is not "routed correctly". Leaving Error rows out of CaseCount would let a + // harness that times out on nine cases in ten report perfect routing accuracy off the one + // that ran. + var repo = new InMemoryRepo { Cases = [RoutingCase("r1"), RoutingCase("r2")] }; + var executor = Build(repo, c => new AgentTestCaseResult + { + CaseId = c.Id, + Status = c.Id == "r1" ? AgentTestStatus.Passed : AgentTestStatus.Error, + Error = c.Id == "r1" ? null : "the case timed out after 120s" + }); + + await executor.ExecuteAsync("run-1", CancellationToken.None); + + var accuracy = Assert.Single(repo.Run.RoutingAccuracies); + Assert.Equal(2, accuracy.CaseCount); + Assert.Equal(1, accuracy.PassedCount); + } + + [Fact] + public async Task Routing_accuracy_is_kept_per_model_so_a_comparison_run_stays_readable() + { + // The whole point of sweeping several models is the difference between them. One run-wide + // figure would average the candidate together with the baseline and hide exactly what the run + // exists to measure. + var repo = new InMemoryRepo { Cases = [RoutingCase("r1"), RoutingCase("r2")] }; + repo.Run.Models = + [ + new TestModel { Provider = "openai", Model = "gpt-4o" }, + new TestModel { Provider = "openai", Model = "gpt-4o-mini" } + ]; + + var (executor, runner) = BuildWithRunner(repo, _ => new AgentTestCaseResult()); + runner.RunWithModel = (c, model) => new AgentTestCaseResult + { + CaseId = c.Id, + Status = model?.Model == "gpt-4o-mini" && c.Id == "r2" + ? AgentTestStatus.Failed + : AgentTestStatus.Passed + }; + + await executor.ExecuteAsync("run-1", CancellationToken.None); + + Assert.Equal(2, repo.Run.RoutingAccuracies.Count); + + var full = repo.Run.RoutingAccuracies.Single(a => a.Model == "gpt-4o"); + Assert.Equal(2, full.CaseCount); + Assert.Equal(2, full.PassedCount); + + var mini = repo.Run.RoutingAccuracies.Single(a => a.Model == "gpt-4o-mini"); + Assert.Equal(2, mini.CaseCount); + Assert.Equal(1, mini.PassedCount); + Assert.Equal("openai", mini.Provider); + } + + [Fact] + public async Task A_run_with_no_routing_cases_records_no_accuracy_rows() + { + // An empty list is what lets a caller tell "this run measured no routing" apart from "this + // run measured routing and got nothing right" -- a 0/0 row would read as the latter. + var repo = new InMemoryRepo { Cases = [CaseNamed("a1")] }; + var executor = Build(repo, c => new AgentTestCaseResult { CaseId = c.Id, Status = AgentTestStatus.Passed }); + + await executor.ExecuteAsync("run-1", CancellationToken.None); + + Assert.Empty(repo.Run.RoutingAccuracies); + } + + private static AgentTestCaseResult Timed(string caseId, long modelMs, long tokens, double cost) => new() + { + CaseId = caseId, + Status = AgentTestStatus.Passed, + ModelDurationMs = modelMs, + TotalTokens = tokens, + Cost = cost + }; + + [Fact] + public async Task Latency_percentiles_are_nearest_rank_and_never_invent_a_duration() + { + // An interpolated percentile returns a number no case actually took, which is indefensible + // the moment someone asks which case was the slow one -- and with the handful of cases a real + // suite starts with, interpolation would be inventing most of the answer. + var durations = new long[] { 100, 200, 300, 400, 500 }; + var repo = new InMemoryRepo + { + Cases = durations.Select((_, i) => CaseNamed($"c{i}")).ToList() + }; + + var byId = durations + .Select((ms, i) => (Id: $"c{i}", Ms: ms)) + .ToDictionary(x => x.Id, x => x.Ms); + + var executor = Build(repo, c => Timed(c.Id, byId[c.Id], tokens: 10, cost: 0.001)); + + await executor.ExecuteAsync("run-1", CancellationToken.None); + + var summary = Assert.Single(repo.Run.PerformanceSummaries); + Assert.Equal(5, summary.CaseCount); + // ceil(0.5 * 5) - 1 = 2 -> the third value. + Assert.Equal(300, summary.LatencyP50Ms); + // ceil(0.95 * 5) - 1 = 4 -> the slowest. + Assert.Equal(500, summary.LatencyP95Ms); + Assert.Contains(summary.LatencyP50Ms, durations); + Assert.Contains(summary.LatencyP95Ms, durations); + } + + [Fact] + public async Task A_case_that_never_reached_the_model_is_left_out_of_the_percentile() + { + // Otherwise a run that mostly crashed reports the best latency anyone has ever seen: the + // failures contribute zero, and zero drags a percentile down hard. + var repo = new InMemoryRepo { Cases = [CaseNamed("ok"), CaseNamed("dead")] }; + var executor = Build(repo, c => c.Id == "ok" + ? Timed("ok", modelMs: 400, tokens: 10, cost: 0.001) + : new AgentTestCaseResult + { + CaseId = "dead", + Status = AgentTestStatus.Error, + Error = "the mock seam is not live", + ModelDurationMs = 0, + TotalTokens = 7, + Cost = 0.0005 + }); + + await executor.ExecuteAsync("run-1", CancellationToken.None); + + var summary = Assert.Single(repo.Run.PerformanceSummaries); + Assert.Equal(1, summary.CaseCount); + Assert.Equal(400, summary.LatencyP50Ms); + + // Tokens and cost still count every result: the failed case spent what it spent, and hiding + // that would understate the run's real cost. + Assert.Equal(17, summary.TotalTokens); + Assert.Equal(0.0015, summary.TotalCost, precision: 6); + } + + [Fact] + public async Task Performance_is_summarised_per_model() + { + // Same reason routing accuracy is: one figure covering every model averages the candidate + // together with the baseline and hides the difference the run exists to measure. + var repo = new InMemoryRepo { Cases = [CaseNamed("a")] }; + repo.Run.Models = + [ + new TestModel { Provider = "openai", Model = "gpt-4o" }, + new TestModel { Provider = "openai", Model = "gpt-4o-mini" } + ]; + + var (executor, runner) = BuildWithRunner(repo, _ => new AgentTestCaseResult()); + runner.RunWithModel = (c, model) => new AgentTestCaseResult + { + CaseId = c.Id, + Status = AgentTestStatus.Passed, + ModelDurationMs = model?.Model == "gpt-4o" ? 900 : 300, + TotalTokens = 100, + Cost = 0.01 + }; + + await executor.ExecuteAsync("run-1", CancellationToken.None); + + Assert.Equal(2, repo.Run.PerformanceSummaries.Count); + Assert.Equal(900, repo.Run.PerformanceSummaries.Single(s => s.Model == "gpt-4o").LatencyP95Ms); + Assert.Equal(300, repo.Run.PerformanceSummaries.Single(s => s.Model == "gpt-4o-mini").LatencyP95Ms); + } + + [Fact] + public async Task The_pricing_behind_a_cost_figure_is_snapshotted() + { + // A cost figure cannot be compared with any other run's without it: a provider price change + // would otherwise surface as a cost regression with nothing to point at. Recording the numbers + // rather than a version string is what makes two runs' pricing checkably the same. + var repo = new InMemoryRepo { Cases = [CaseNamed("a")] }; + repo.Run.Models = [new TestModel { Provider = "openai", Model = "gpt-4o" }]; + + var (executor, _) = BuildWithRunner( + repo, + c => Timed(c.Id, 100, 50, 0.005), + PricingFor("openai", "gpt-4o", input: 2.5f, output: 10f)); + + await executor.ExecuteAsync("run-1", CancellationToken.None); + + var pricing = Assert.Single(repo.Run.ModelPricing); + Assert.Equal("gpt-4o", pricing.Model); + Assert.Equal(2.5f, pricing.TextInputCost); + Assert.Equal(10f, pricing.TextOutputCost); + } + + [Fact] + public async Task Unknown_pricing_is_recorded_as_unknown_rather_than_as_free() + { + // Zero would read as "this model costs nothing", which is a claim. Null reads as "nobody + // knows", which is the truth and stops the figure being compared. + var repo = new InMemoryRepo { Cases = [CaseNamed("a")] }; + repo.Run.Models = [new TestModel { Provider = "openai", Model = "gpt-9-imaginary" }]; + + var (executor, _) = BuildWithRunner( + repo, c => Timed(c.Id, 100, 50, 0.005), PricingFor("openai", "gpt-4o", 2.5f, 10f)); + + await executor.ExecuteAsync("run-1", CancellationToken.None); + + var pricing = Assert.Single(repo.Run.ModelPricing); + Assert.Null(pricing.TextInputCost); + Assert.Null(pricing.TextOutputCost); + } } diff --git a/tests/BotSharp.Core.UnitTests/AgentTesting/AssertionEvaluatorTests.cs b/tests/BotSharp.Core.UnitTests/AgentTesting/AssertionEvaluatorTests.cs index fc9f01c11..88b911bcf 100644 --- a/tests/BotSharp.Core.UnitTests/AgentTesting/AssertionEvaluatorTests.cs +++ b/tests/BotSharp.Core.UnitTests/AgentTesting/AssertionEvaluatorTests.cs @@ -24,7 +24,10 @@ private static AssertionContext Context( Output = output, ToolCalls = calls ?? [], States = states ?? new Dictionary(), - RoutedToAgent = routedTo + // routedToAgent reads the chain's last entry, so a one-hop chain IS "routed to this + // agent". Given a name only, the id is left blank -- which is also what an agent whose + // record cannot be loaded looks like. + AgentChain = routedTo == null ? [] : [new AgentChainHop { Id = string.Empty, Name = routedTo }] }; [Fact] @@ -170,14 +173,18 @@ public void An_unknown_assertion_type_fails_loudly() } [Fact] - public void Llm_judge_is_reported_as_unavailable_in_p1_rather_than_silently_passing() + public void Llm_judge_fails_loudly_here_rather_than_silently_passing() { + // llmJudge is scored by IAgentTestJudge, because it needs a model call and this evaluator is + // pure and synchronous. Reaching this branch means somebody evaluated assertions without + // going through the runner, and the verdict they get back must be one they cannot mistake + // for a pass -- a silent pass would show a case that verified nothing as green. var result = AssertionEvaluator.Evaluate( - new TestAssertion { Type = AssertionTypes.LlmJudge, Expected = "confirms the address before quoting", MinScore = 0.8 }, + new TestAssertion { Type = AssertionTypes.LlmJudge, Expected = "confirms the address before quoting", MinScore = 4 }, Context(output: "whatever")); Assert.False(result.Passed); - Assert.Contains("not available in P1", result.Message!); + Assert.Contains("IAgentTestJudge", result.Message!); } /// diff --git a/tests/BotSharp.Core.UnitTests/AgentTesting/CaseScopeTests.cs b/tests/BotSharp.Core.UnitTests/AgentTesting/CaseScopeTests.cs new file mode 100644 index 000000000..89855118b --- /dev/null +++ b/tests/BotSharp.Core.UnitTests/AgentTesting/CaseScopeTests.cs @@ -0,0 +1,245 @@ +using System.Collections.Generic; +using System.Linq; +using BotSharp.Plugin.AgentTesting.Models; +using BotSharp.Plugin.AgentTesting.Services; +using Xunit; + +namespace BotSharp.Core.UnitTests.AgentTesting; + +/// +/// Scope narrowing is the one feature here whose failure mode is silence. A case wrongly INCLUDED +/// costs tokens and is obvious. A case wrongly EXCLUDED produces no result at all, and once the +/// numbers reach a report "not run" is indistinguishable from "passed" -- so every rule below is +/// checked in the direction of including, and the exclusions are checked for having a reason someone +/// can review. +/// +public class CaseScopeTests +{ + private const string CopilotId = "2cd4b805-7078-4405-87e9-2ec9aadf8a11"; + private const string WoCancellationId = "0fe3905d-75f1-4e2e-8e54-ec3d33d6b6f0"; + private const string DiagnosisId = "11111111-2222-3333-4444-555555555555"; + + private static AgentTestCase Case( + string? entryAgentId = null, + bool enabled = true, + bool crossCutting = false, + string priority = CasePriorities.P1, + int? batch = null, + params string[] involved) => new() + { + Id = "case-1", + SuiteId = "suite-1", + Name = "c", + Enabled = enabled, + CrossCutting = crossCutting, + Priority = priority, + Batch = batch, + EntryAgentId = entryAgentId, + InvolvedAgents = involved.ToList(), + Turns = [new TestTurn { Index = 0, UserMessage = "hi" }] + }; + + private static ScopeQuery Changed(params string[] targetAgentIds) + => new() { TargetAgentIds = targetAgentIds }; + + // ------------------------------------------------------------------ involved agents + + [Fact] + public void An_authored_involved_list_wins_over_the_entry_agent() + { + // The reason the field exists: for a routing case the entry agent is the router, and the + // agents that matter are the ones downstream of it. Those cannot be derived from the case + // definition at all. + var involved = CaseScope.InvolvedAgentIds( + Case(entryAgentId: CopilotId, involved: [WoCancellationId, DiagnosisId]), "suite-agent"); + + Assert.Equal([WoCancellationId, DiagnosisId], involved); + } + + [Fact] + public void With_no_authored_list_the_entry_agent_is_the_involved_agent() + { + // Definitionally true and already known, so an Agent case is picked up by a change to the + // agent it runs against without anyone maintaining a list. + Assert.Equal([CopilotId], CaseScope.InvolvedAgentIds(Case(entryAgentId: CopilotId), "suite-agent")); + } + + [Fact] + public void With_no_entry_agent_the_suites_agent_is_used() + { + // Which is exactly what the runner does when it opens the conversation, so the scope is + // computed against the agent the case really runs on. + Assert.Equal(["suite-agent"], CaseScope.InvolvedAgentIds(Case(), "suite-agent")); + } + + [Fact] + public void Blank_entries_in_an_authored_list_are_ignored() + { + // A UI that posts an empty row must not contribute an agent id of "" that matches nothing and + // silently narrows the scope. + var involved = CaseScope.InvolvedAgentIds( + Case(involved: [CopilotId, "", " "]), "suite-agent"); + + Assert.Equal([CopilotId], involved); + } + + // ------------------------------------------------------------------ rules + + [Fact] + public void Rule_3_includes_a_case_that_touches_a_changed_agent() + { + var decision = CaseScope.Decide(Case(entryAgentId: CopilotId), "suite-agent", Changed(CopilotId)); + + Assert.True(decision.Included); + Assert.Equal(ScopeReasons.TargetAgent, decision.Reason); + } + + [Fact] + public void Rule_4_excludes_a_case_that_touches_none_of_them() + { + // The entire point of narrowing: a change to one agent cannot affect a case that never goes + // near it, and running it anyway buys nothing. + var decision = CaseScope.Decide(Case(entryAgentId: CopilotId), "suite-agent", Changed(DiagnosisId)); + + Assert.False(decision.Included); + Assert.Equal(ScopeReasons.NotInvolved, decision.Reason); + } + + [Fact] + public void Agent_ids_are_matched_case_insensitively() + { + var decision = CaseScope.Decide( + Case(entryAgentId: CopilotId.ToUpperInvariant()), "suite-agent", Changed(CopilotId)); + + Assert.True(decision.Included); + } + + [Fact] + public void Rule_1_includes_a_cross_cutting_case_no_matter_what_changed() + { + // Narrowing exists to skip cases a change cannot affect, and "this change cannot affect + // safety" is precisely the claim not to accept untested. + var decision = CaseScope.Decide( + Case(entryAgentId: CopilotId, crossCutting: true), "suite-agent", Changed(DiagnosisId)); + + Assert.True(decision.Included); + Assert.Equal(ScopeReasons.CrossCutting, decision.Reason); + } + + [Fact] + public void Rule_2_includes_everything_for_a_platform_wide_change() + { + // A foundation model or provider swap has no agent it demonstrably does not touch, so + // narrowing switches off rather than being applied against an empty target list. + var decision = CaseScope.Decide( + Case(entryAgentId: CopilotId), "suite-agent", + new ScopeQuery { FullPlatform = true }); + + Assert.True(decision.Included); + Assert.Equal(ScopeReasons.FullPlatform, decision.Reason); + } + + [Fact] + public void A_case_with_no_known_agents_is_included_rather_than_dropped() + { + // Fail open. An unknown involved set means the harness cannot show the change does not affect + // this case; excluding it on that basis would produce no result to notice. + var decision = CaseScope.Decide(Case(), suiteAgentId: null, Changed(CopilotId)); + + Assert.True(decision.Included); + Assert.Equal(ScopeReasons.UnknownAgents, decision.Reason); + } + + [Fact] + public void A_disabled_case_is_excluded_and_says_so() + { + // The executor skips it, so calling it in scope would overstate coverage by exactly the cases + // nobody is running. Its own reason, so a disabled cross-cutting safety case stands out + // instead of blending in with cases the change genuinely cannot affect. + var decision = CaseScope.Decide( + Case(entryAgentId: CopilotId, enabled: false, crossCutting: true), "suite-agent", + Changed(CopilotId)); + + Assert.False(decision.Included); + Assert.Equal(ScopeReasons.Disabled, decision.Reason); + } + + // ------------------------------------------------------------------ batches + + [Theory] + [InlineData(CasePriorities.P0, CaseBatches.StopLoss)] + [InlineData(CasePriorities.P1, CaseBatches.Mandatory)] + [InlineData(CasePriorities.P2, CaseBatches.Optional)] + public void Priority_derives_the_batch(string priority, int expected) + { + Assert.Equal(expected, CaseBatches.Effective(Case(priority: priority))); + } + + [Fact] + public void A_cross_cutting_case_is_batch_one_whatever_its_priority() + { + // A safety case that only runs once everything else has passed cannot stop anything, which is + // the entire job of batch 1. + Assert.Equal( + CaseBatches.StopLoss, + CaseBatches.Effective(Case(priority: CasePriorities.P2, crossCutting: true))); + } + + [Fact] + public void An_explicit_batch_overrides_both() + { + Assert.Equal( + CaseBatches.Optional, + CaseBatches.Effective(Case(priority: CasePriorities.P0, crossCutting: true, batch: CaseBatches.Optional))); + } + + [Fact] + public void An_out_of_range_explicit_batch_falls_back_to_the_derivation() + { + // Rejected at save time, so this is only reachable for a document written before the check or + // edited around the API. Falling back beats filing the case in a batch that does not exist, + // where nothing would ever run it. + Assert.Equal(CaseBatches.StopLoss, CaseBatches.Effective(Case(priority: CasePriorities.P0, batch: 9))); + } + + [Fact] + public void Narrowing_to_a_batch_excludes_the_others_with_their_own_reason() + { + // Scheduling, not scoping: batches exist to run in order and stop early, so a case left out + // of this batch is not out of scope -- it just runs later, and the reason has to say that. + var decision = CaseScope.Decide( + Case(entryAgentId: CopilotId, priority: CasePriorities.P2), "suite-agent", + new ScopeQuery { TargetAgentIds = [CopilotId], Batch = CaseBatches.StopLoss }); + + Assert.False(decision.Included); + Assert.Equal(ScopeReasons.OtherBatch, decision.Reason); + } + + [Fact] + public void A_cross_cutting_case_still_belongs_to_batch_one_when_narrowing_by_batch() + { + // The two axes have to agree: cross-cutting forces batch 1, so asking for batch 1 has to + // return it. If they disagreed, the stop-loss batch would run without its safety cases. + var decision = CaseScope.Decide( + Case(entryAgentId: DiagnosisId, crossCutting: true, priority: CasePriorities.P2), + "suite-agent", + new ScopeQuery { TargetAgentIds = [CopilotId], Batch = CaseBatches.StopLoss }); + + Assert.True(decision.Included); + Assert.Equal(ScopeReasons.CrossCutting, decision.Reason); + Assert.Equal(CaseBatches.StopLoss, decision.Batch); + } + + [Fact] + public void The_decision_reports_the_set_it_was_made_against() + { + // A scope nobody can explain is a scope nobody can review, and reviewing it is the only + // defence against signing off a change against a set that quietly left out the interesting + // case. + var decision = CaseScope.Decide( + Case(entryAgentId: CopilotId, involved: [WoCancellationId]), "suite-agent", Changed(CopilotId)); + + Assert.False(decision.Included); + Assert.Equal([WoCancellationId], decision.InvolvedAgentIds); + } +} diff --git a/tests/BotSharp.Core.UnitTests/AgentTesting/SyntheticConversationExemptionTests.cs b/tests/BotSharp.Core.UnitTests/AgentTesting/SyntheticConversationExemptionTests.cs new file mode 100644 index 000000000..891bdd31b --- /dev/null +++ b/tests/BotSharp.Core.UnitTests/AgentTesting/SyntheticConversationExemptionTests.cs @@ -0,0 +1,280 @@ +using System; +using System.Collections.Generic; +using System.Threading.Tasks; +using BotSharp.Abstraction.Agents.Enums; +using BotSharp.Abstraction.Conversations; +using BotSharp.Abstraction.Conversations.Models; +using BotSharp.Abstraction.Conversations.Settings; +using BotSharp.Abstraction.Repositories.Filters; +using BotSharp.Abstraction.Users; +using BotSharp.Abstraction.Utilities; +using BotSharp.Logger.Hooks; +using BotSharp.Plugin.AgentTesting.Runtime; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging.Abstractions; +using Moq; +using Xunit; + +namespace BotSharp.Core.UnitTests.AgentTesting; + +/// +/// BotSharp's rate limiting counts human behaviour, and a regression suite does not look like one: it +/// opens a conversation per case per model and drives the turns as fast as the model answers. +/// +/// Two of its three guards therefore have to stand aside for a harness conversation, and BOTH of them +/// matter -- the conversation quota is the one that surfaced first, but the two-second gap between +/// messages blocks any case with authored history outright, because injected history lands with the +/// current timestamp and the first real turn follows it immediately. +/// +/// The direction of these tests matters as much as the assertions: a probe that wrongly answers yes +/// exempts real user traffic from the limits it is meant to be held to, so every "real traffic" case +/// below is checked for still being limited. +/// +public class SyntheticConversationExemptionTests +{ + private const string ConversationId = "conv-under-test"; + + private static RateLimitConversationHook BuildHook( + IServiceProvider services, List dialogs) + { + var hook = new RateLimitConversationHook( + services, NullLogger.Instance); + + // Dialogs is only reachable through the base class's own loader, which is also how the real + // conversation path populates it. + hook.OnDialogsLoaded(dialogs).GetAwaiter().GetResult(); + return hook; + } + + /// + /// Everything OnMessageReceived reaches for. null registers none at all, + /// which is the normal deployment and must exempt nobody. + /// + private static IServiceProvider BuildServices( + ISyntheticConversationProbe? probe, + int conversationsToday, + string conversationId = ConversationId) + { + var services = new ServiceCollection(); + + services.AddSingleton(new ConversationSetting + { + RateLimit = new RateLimitSetting + { + MaxConversationPerDay = 100, + MaxInputLengthPerRequest = 1024, + MinTimeSecondsBetweenMessages = 2 + } + }); + + var states = new Mock(); + // Blank, exactly as a harness conversation leaves it: the guards are skipped for Phone, Email + // and Database channels, and the harness deliberately seeds no channel because production + // routing branches on that key. + states.Setup(x => x.GetState(It.IsAny(), It.IsAny())).Returns(string.Empty); + services.AddSingleton(states.Object); + + var identity = new Mock(); + // Empty, as it is in a BackgroundService with no HTTP context. Note the Mongo filter drops an + // empty UserId rather than matching on it, so this counts every conversation in the instance. + identity.Setup(x => x.Id).Returns(string.Empty); + services.AddSingleton(identity.Object); + + var conversations = new Mock(); + conversations.Setup(x => x.ConversationId).Returns(conversationId); + conversations + .Setup(x => x.GetConversations(It.IsAny())) + .ReturnsAsync(new PagedItems + { + Count = conversationsToday, + Items = [] + }); + services.AddSingleton(conversations.Object); + + if (probe != null) + { + services.AddSingleton(probe); + } + + return services.BuildServiceProvider(); + } + + private static List TwoUserMessagesOneSecondApart() => + [ + new RoleDialogModel(AgentRole.User, "earlier") { CreatedAt = DateTime.UtcNow.AddSeconds(-1) }, + new RoleDialogModel(AgentRole.User, "now") { CreatedAt = DateTime.UtcNow } + ]; + + // ------------------------------------------------------------------ the probe + + [Fact] + public void The_probe_recognises_a_conversation_the_harness_registered() + { + var registry = new AgentTestRunRegistry(); + registry.Register(new ActiveTestRun { ConversationId = ConversationId, CaseId = "case-1" }); + + var probe = new AgentTestSyntheticConversationProbe(registry); + + Assert.True(probe.IsSynthetic(ConversationId)); + } + + [Theory] + [InlineData("some-other-conversation")] + [InlineData("")] + [InlineData(null)] + public void The_probe_answers_no_for_anything_it_does_not_recognise(string? conversationId) + { + // Answering yes here would exempt real user traffic from the limits it exists to be held to, + // which is a far worse failure than a test being rate limited. + var registry = new AgentTestRunRegistry(); + registry.Register(new ActiveTestRun { ConversationId = ConversationId, CaseId = "case-1" }); + + var probe = new AgentTestSyntheticConversationProbe(registry); + + Assert.False(probe.IsSynthetic(conversationId!)); + } + + [Fact] + public void The_probe_stops_recognising_a_conversation_once_the_case_finishes() + { + // The runner unregisters in a finally block. If the probe kept saying yes afterwards, that + // conversation id would stay exempt from rate limiting for the life of the process. + var registry = new AgentTestRunRegistry(); + registry.Register(new ActiveTestRun { ConversationId = ConversationId, CaseId = "case-1" }); + var probe = new AgentTestSyntheticConversationProbe(registry); + + registry.Unregister(ConversationId); + + Assert.False(probe.IsSynthetic(ConversationId)); + } + + // ------------------------------------------------------------------ the quota guard + + [Fact] + public async Task Real_traffic_over_the_quota_is_still_stopped() + { + // The guard has to keep working. With no probe registered -- the normal deployment -- nothing + // is exempt. + var services = BuildServices(probe: null, conversationsToday: 107); + var hook = BuildHook(services, []); + var message = new RoleDialogModel(AgentRole.User, "hello"); + + await hook.OnMessageReceived(message); + + Assert.True(message.StopCompletion); + Assert.Contains("exceeds the system maximum of 100", message.Content); + } + + [Fact] + public async Task A_harness_conversation_over_the_quota_runs_anyway() + { + // The reported failure. 107 conversations across the whole instance in 24 hours, of which the + // harness opened 30 -- and because an empty UserId drops the filter rather than matching on + // it, the harness was measured against all 107 and every case failed. + var registry = new AgentTestRunRegistry(); + registry.Register(new ActiveTestRun { ConversationId = ConversationId, CaseId = "case-1" }); + + var services = BuildServices( + new AgentTestSyntheticConversationProbe(registry), conversationsToday: 107); + var hook = BuildHook(services, []); + var message = new RoleDialogModel(AgentRole.User, "hello"); + + await hook.OnMessageReceived(message); + + Assert.False(message.StopCompletion); + Assert.Equal("hello", message.Content); + } + + [Fact] + public async Task A_conversation_the_probe_does_not_claim_is_still_limited() + { + // A harness being active must not exempt everything else running at the same time. + var registry = new AgentTestRunRegistry(); + registry.Register(new ActiveTestRun { ConversationId = "a-different-case", CaseId = "case-1" }); + + var services = BuildServices( + new AgentTestSyntheticConversationProbe(registry), conversationsToday: 107); + var hook = BuildHook(services, []); + var message = new RoleDialogModel(AgentRole.User, "hello"); + + await hook.OnMessageReceived(message); + + Assert.True(message.StopCompletion); + } + + // ------------------------------------------------------------------ the frequency guard + + [Fact] + public async Task Real_traffic_sending_faster_than_the_minimum_gap_is_still_stopped() + { + var services = BuildServices(probe: null, conversationsToday: 1); + var hook = BuildHook(services, TwoUserMessagesOneSecondApart()); + var message = new RoleDialogModel(AgentRole.User, "hello"); + + await hook.OnMessageReceived(message); + + Assert.True(message.StopCompletion); + Assert.Contains("frequency", message.Content); + } + + [Fact] + public async Task A_harness_conversation_is_not_held_to_the_minimum_gap() + { + // This is the guard that blocks any case with authored history: injected history lands with + // the current timestamp, so the first real turn follows the last history message by about zero + // seconds and trips a two-second minimum every time. It would also make multi-turn cases + // flaky whenever the model happens to answer quickly. + var registry = new AgentTestRunRegistry(); + registry.Register(new ActiveTestRun { ConversationId = ConversationId, CaseId = "case-1" }); + + var services = BuildServices( + new AgentTestSyntheticConversationProbe(registry), conversationsToday: 1); + var hook = BuildHook(services, TwoUserMessagesOneSecondApart()); + var message = new RoleDialogModel(AgentRole.User, "hello"); + + await hook.OnMessageReceived(message); + + Assert.False(message.StopCompletion); + } + + // ------------------------------------------------------------------ the input length guard + + [Fact] + public async Task An_over_long_message_is_still_rejected_in_a_harness_conversation() + { + // Kept in force on purpose. That guard is about one message being too large for the model, + // which is a real condition a test should surface rather than be excused from -- unlike the + // two volume guards, which measure how much a human has been using the system. + var registry = new AgentTestRunRegistry(); + registry.Register(new ActiveTestRun { ConversationId = ConversationId, CaseId = "case-1" }); + + var services = BuildServices( + new AgentTestSyntheticConversationProbe(registry), conversationsToday: 1); + var hook = BuildHook(services, []); + var message = new RoleDialogModel(AgentRole.User, new string('x', 2000)); + + await hook.OnMessageReceived(message); + + Assert.True(message.StopCompletion); + Assert.Contains("characters", message.Content); + } + + // ------------------------------------------------------------------ a misbehaving probe + + [Fact] + public async Task A_probe_that_throws_fails_closed() + { + // Failing open would let a bug in a probe silently lift the limits for real traffic. The worst + // case here is a harness message being rate limited, which is visible and recoverable. + var probe = new Mock(); + probe.Setup(x => x.IsSynthetic(It.IsAny())).Throws(new InvalidOperationException("boom")); + + var services = BuildServices(probe.Object, conversationsToday: 107); + var hook = BuildHook(services, []); + var message = new RoleDialogModel(AgentRole.User, "hello"); + + await hook.OnMessageReceived(message); + + Assert.True(message.StopCompletion); + } +} diff --git a/tests/BotSharp.Core.UnitTests/BotSharp.Core.UnitTests.csproj b/tests/BotSharp.Core.UnitTests/BotSharp.Core.UnitTests.csproj index 4dd287f90..030ffeaa7 100644 --- a/tests/BotSharp.Core.UnitTests/BotSharp.Core.UnitTests.csproj +++ b/tests/BotSharp.Core.UnitTests/BotSharp.Core.UnitTests.csproj @@ -29,6 +29,8 @@ + + From 46a811656ca5aed17c565997afd3c69155b24aa4 Mon Sep 17 00:00:00 2001 From: "mars.yu" Date: Fri, 21 Aug 2026 10:10:54 +0800 Subject: [PATCH 2/3] Clear run history: select-all and delete Runs accumulate forever -- one per case per model per run, and nothing ever removed them. Adds a way to clear them, with the guards the operation needs. POST runs/delete takes a list. Bulk-only, and the single-row button calls it with one id: a separate DELETE runs/{id} would be a second path to the same destructive operation, and the guard below is worth having in exactly one place. Deleting a run deletes its case results too. Results are keyed by run id and reachable no other way, so dropping the run alone would leave rows nothing can ever list, read or clean up again -- and every later aggregate that scans results would keep counting them. Results go first, so a process death between the two deletes leaves an orphaned RUN, which is visible and deletable again, rather than orphaned RESULTS, which are not reachable at all. A run that is still executing is refused. Deleting it would not stop it: the queue keeps driving cases, keeps spending tokens, and keeps writing results for a run id that no longer exists. Cancel it first -- and on a live row the UI offers only Cancel, since the other button would not do what it says. One live run does not fail the batch. Selecting everything and clearing is the normal way this gets used, and a running row in the list is common; refusing the whole call would make the feature useless exactly when it is most wanted. Skipped runs come back with a reason and the UI shows each one, because "deleted 2" while a third row silently stays is how someone concludes the button is broken. The count on the button excludes running runs, so it never promises a delete that will not happen. Behind the same admin gate as triggering a run: runs are the record of whether an agent change was evaluated at all, so removing them is at least as consequential as creating them. An empty list is a 400 rather than a successful no-op -- far more likely a select-all that selected nothing than a deliberate request. Not touched: the conversations these runs created. They live in BotSharp's own store and are the only forensic record of what an agent actually said when an assertion failed, so they are not something to remove as a side effect of tidying a list. The harness still never cleans them up, which is a separate decision to make. 8 tests: the cascade, the running-run refusal, one live run not blocking a batch, an already-deleted run being reported rather than erroring, duplicate ids deleting once, two shapes of empty request, and the admin gate. 344 unit tests pass. Co-Authored-By: Claude Opus 5 --- .../Controllers/AgentTestController.cs | 63 +++++++ .../Models/AgentTestDtos.cs | 33 ++++ .../Repositories/AgentTestRepository.cs | 20 ++ .../AgentTesting/AgentTestControllerTests.cs | 172 +++++++++++++++++- .../AgentTesting/AgentTestRunExecutorTests.cs | 10 + 5 files changed, 297 insertions(+), 1 deletion(-) diff --git a/src/Plugins/BotSharp.Plugin.AgentTesting/Controllers/AgentTestController.cs b/src/Plugins/BotSharp.Plugin.AgentTesting/Controllers/AgentTestController.cs index 54afeb01a..145e2d738 100644 --- a/src/Plugins/BotSharp.Plugin.AgentTesting/Controllers/AgentTestController.cs +++ b/src/Plugins/BotSharp.Plugin.AgentTesting/Controllers/AgentTestController.cs @@ -356,6 +356,69 @@ private static string NextCopyName(string name, IEnumerable existing) } } + /// + /// Clears run history: removes the named runs and the case results underneath them. + /// + /// Bulk-only, and the single-row button in the UI calls it with one id. A separate + /// DELETE runs/{id} would be a second path to the same destructive operation, and the guard + /// below is one of the things worth having in exactly one place. + /// + /// Behind the same admin gate as triggering a run. Runs are the audit trail for whether an agent + /// change was evaluated at all, so removing them is at least as consequential as creating them. + /// + [BotSharpAuth] + [HttpPost("runs/delete")] + public async Task> DeleteRuns( + [FromBody] AgentTestRunDeleteRequest request) + { + var runIds = (request?.RunIds ?? []) + .Where(id => !string.IsNullOrWhiteSpace(id)) + .Select(id => id.Trim()) + .Distinct(StringComparer.Ordinal) + .ToList(); + + if (runIds.Count == 0) + { + return BadRequest("name at least one run to delete"); + } + + var response = new AgentTestRunDeleteResponse(); + + foreach (var runId in runIds) + { + var run = await _repo.GetRunAsync(runId); + if (run == null) + { + // Already gone. Reported rather than treated as an error: two people clearing the + // same history is a normal race, not a failure worth refusing the whole batch over. + response.Skipped.Add(new SkippedRunDto + { + RunId = runId, + Reason = "already deleted" + }); + continue; + } + + if (!IsTerminalStatus(run.Status)) + { + // Deleting a run that is still executing does not stop it: the queue keeps driving + // cases, keeps spending tokens, and keeps writing results for a run id that no longer + // exists -- results nothing can ever list again. Cancel first. + response.Skipped.Add(new SkippedRunDto + { + RunId = runId, + Reason = $"the run is {run.Status.ToLowerInvariant()}; cancel it before deleting" + }); + continue; + } + + response.DeletedResultCount += await _repo.DeleteRunAsync(runId); + response.DeletedRunIds.Add(runId); + } + + return response; + } + [HttpDelete("cases/{id}")] public async Task DeleteCase(string id) { diff --git a/src/Plugins/BotSharp.Plugin.AgentTesting/Models/AgentTestDtos.cs b/src/Plugins/BotSharp.Plugin.AgentTesting/Models/AgentTestDtos.cs index 0ca4f1ad3..ddac1975f 100644 --- a/src/Plugins/BotSharp.Plugin.AgentTesting/Models/AgentTestDtos.cs +++ b/src/Plugins/BotSharp.Plugin.AgentTesting/Models/AgentTestDtos.cs @@ -213,6 +213,39 @@ public class AgentTestRunTriggerRequest public List? Models { get; set; } } +/// +/// Body of POST /agent-test/runs/delete -- clear run history. +/// +public class AgentTestRunDeleteRequest +{ + public List RunIds { get; set; } = []; +} + +/// +/// What a delete actually did. +/// +/// Reports the skipped runs rather than failing the whole call: a bulk delete over a list that +/// happens to include a still-running row should remove the rest, and the caller needs to be able to +/// say WHY one survived instead of leaving the user to notice a row that quietly stayed. +/// +public class AgentTestRunDeleteResponse +{ + public List DeletedRunIds { get; set; } = []; + + /// Case results removed along with those runs. + public long DeletedResultCount { get; set; } + + public List Skipped { get; set; } = []; +} + +public class SkippedRunDto +{ + public string RunId { get; set; } = string.Empty; + + /// Caller-facing text, already explaining what to do about it. + public string Reason { get; set; } = string.Empty; +} + /// /// Body of GET /agent-test/runs/{id}: one run plus every AgentTestCaseResult belonging to it. /// diff --git a/src/Plugins/BotSharp.Plugin.AgentTesting/Repositories/AgentTestRepository.cs b/src/Plugins/BotSharp.Plugin.AgentTesting/Repositories/AgentTestRepository.cs index d2dcbeb88..4773ebde9 100644 --- a/src/Plugins/BotSharp.Plugin.AgentTesting/Repositories/AgentTestRepository.cs +++ b/src/Plugins/BotSharp.Plugin.AgentTesting/Repositories/AgentTestRepository.cs @@ -34,6 +34,16 @@ public interface IAgentTestRepository Task UpdateRunAsync(AgentTestRun run); + /// + /// Removes a run and every case result belonging to it, and reports how many results went with + /// it. + /// + /// The cascade is the point. Case results are keyed by run id and are reachable no other way, so + /// deleting the run alone would leave rows nothing can ever list, read or clean up again -- and + /// every later aggregate that scans results would keep counting them. + /// + Task DeleteRunAsync(string id); + Task AddCaseResultAsync(AgentTestCaseResult result); Task> ListCaseResultsAsync(string runId); } @@ -149,6 +159,16 @@ public async Task> ListRunsByStatusAsync(string status) public async Task UpdateRunAsync(AgentTestRun run) => await _mongoDbContext.AgentTestRuns.ReplaceOneAsync(x => x.Id == run.Id, run); + public async Task DeleteRunAsync(string id) + { + // Results first. If the process dies between the two deletes, an orphaned RUN is visible and + // deletable again; orphaned RESULTS are not reachable at all once their run is gone. Losing + // the recoverable half is the better failure. + var results = await _mongoDbContext.AgentTestCaseResults.DeleteManyAsync(x => x.RunId == id); + await _mongoDbContext.AgentTestRuns.DeleteOneAsync(x => x.Id == id); + return results.DeletedCount; + } + public async Task AddCaseResultAsync(AgentTestCaseResult result) { if (string.IsNullOrEmpty(result.Id)) diff --git a/tests/BotSharp.Core.UnitTests/AgentTesting/AgentTestControllerTests.cs b/tests/BotSharp.Core.UnitTests/AgentTesting/AgentTestControllerTests.cs index 165e21478..3a6bf8640 100644 --- a/tests/BotSharp.Core.UnitTests/AgentTesting/AgentTestControllerTests.cs +++ b/tests/BotSharp.Core.UnitTests/AgentTesting/AgentTestControllerTests.cs @@ -86,8 +86,26 @@ public Task> ListRunsByStatusAsync(string status) => Task.FromResult(Runs.Values.Where(r => r.Status == status).ToList()); public Task UpdateRunAsync(AgentTestRun run) { Runs[run.Id] = run; return Task.CompletedTask; } + /// + /// Cascades exactly as the real repository does. A fake that dropped only the run would let + /// a test pass while production left unreachable result rows behind. + /// + public Task DeleteRunAsync(string id) + { + var removed = Results.RemoveAll(r => r.RunId == id); + Runs.Remove(id); + return Task.FromResult((long)removed); + } + public Task AddCaseResultAsync(AgentTestCaseResult result) => Task.CompletedTask; - public Task> ListCaseResultsAsync(string runId) => Task.FromResult(new List()); + /// + /// Real storage rather than an always-empty list, so the delete cascade is actually + /// observable: a fake that never held results could not tell a cascade from a no-op. + /// + public List Results { get; } = []; + + public Task> ListCaseResultsAsync(string runId) + => Task.FromResult(Results.Where(r => r.RunId == runId).ToList()); } private sealed class RecordingQueue : IAgentTestRunQueue @@ -1465,4 +1483,156 @@ public async Task An_out_of_range_batch_on_a_scope_is_rejected() Assert.IsType(response.Result); } + + // ------------------------------------------------------------- clearing run history + + private static InMemoryRepo RepoWithRuns() + { + var repo = RepoWithSuite(); + repo.Runs["done"] = new AgentTestRun + { + Id = "done", SuiteId = "suite-1", Status = AgentTestStatus.Passed + }; + repo.Runs["failed"] = new AgentTestRun + { + Id = "failed", SuiteId = "suite-1", Status = AgentTestStatus.Failed + }; + repo.Runs["live"] = new AgentTestRun + { + Id = "live", SuiteId = "suite-1", Status = AgentTestStatus.Running + }; + repo.Results.Add(new AgentTestCaseResult { Id = "r1", RunId = "done", CaseId = "c1" }); + repo.Results.Add(new AgentTestCaseResult { Id = "r2", RunId = "done", CaseId = "c2" }); + repo.Results.Add(new AgentTestCaseResult { Id = "r3", RunId = "live", CaseId = "c1" }); + return repo; + } + + [Fact] + public async Task Deleting_a_run_takes_its_case_results_with_it() + { + // Results are keyed by run id and reachable no other way, so dropping the run alone would + // leave rows nothing can ever list, read or clean up again. + var repo = RepoWithRuns(); + var controller = BuildController(repo, out _); + + var response = await controller.DeleteRuns(new AgentTestRunDeleteRequest { RunIds = ["done"] }); + + var result = Assert.IsType(response.Value); + Assert.Equal(["done"], result.DeletedRunIds); + Assert.Equal(2, result.DeletedResultCount); + Assert.DoesNotContain("done", repo.Runs.Keys); + Assert.DoesNotContain(repo.Results, r => r.RunId == "done"); + + // And it touched nothing else. + Assert.Contains(repo.Results, r => r.RunId == "live"); + } + + [Fact] + public async Task A_running_run_is_refused_rather_than_deleted() + { + // Deleting it would not stop it: the queue keeps driving cases, keeps spending tokens, and + // keeps writing results for a run id that no longer exists. + var repo = RepoWithRuns(); + var controller = BuildController(repo, out _); + + var response = await controller.DeleteRuns(new AgentTestRunDeleteRequest { RunIds = ["live"] }); + + var result = Assert.IsType(response.Value); + Assert.Empty(result.DeletedRunIds); + Assert.Contains("cancel it before deleting", Assert.Single(result.Skipped).Reason); + Assert.Contains("live", repo.Runs.Keys); + Assert.Contains(repo.Results, r => r.RunId == "live"); + } + + [Fact] + public async Task One_running_run_does_not_block_the_rest_of_the_batch() + { + // Selecting everything and clearing is the normal way this is used, and a live run in the + // list is common. Refusing the whole call would make the feature unusable exactly when it is + // most wanted. + var repo = RepoWithRuns(); + var controller = BuildController(repo, out _); + + var response = await controller.DeleteRuns(new AgentTestRunDeleteRequest + { + RunIds = ["done", "live", "failed"] + }); + + var result = Assert.IsType(response.Value); + Assert.Equal(["done", "failed"], result.DeletedRunIds); + Assert.Equal("live", Assert.Single(result.Skipped).RunId); + Assert.Equal(["live"], repo.Runs.Keys.ToList()); + } + + [Fact] + public async Task A_run_that_is_already_gone_is_reported_not_an_error() + { + // Two people clearing the same history is a race, not a failure worth refusing a batch over. + var repo = RepoWithRuns(); + var controller = BuildController(repo, out _); + + var response = await controller.DeleteRuns(new AgentTestRunDeleteRequest + { + RunIds = ["done", "never-existed"] + }); + + var result = Assert.IsType(response.Value); + Assert.Equal(["done"], result.DeletedRunIds); + Assert.Equal("already deleted", Assert.Single(result.Skipped).Reason); + } + + [Fact] + public async Task Duplicate_ids_are_deleted_once() + { + // A select-all plus a row click can send the same id twice; the second pass would otherwise + // report it as "already deleted" and make the summary read as a partial failure. + var repo = RepoWithRuns(); + var controller = BuildController(repo, out _); + + var response = await controller.DeleteRuns(new AgentTestRunDeleteRequest + { + RunIds = ["done", "done"] + }); + + var result = Assert.IsType(response.Value); + Assert.Equal(["done"], result.DeletedRunIds); + Assert.Empty(result.Skipped); + } + + [Fact] + public async Task An_empty_delete_is_rejected() + { + // An empty list is far more likely to be a UI bug -- a select-all that selected nothing -- + // than a deliberate no-op, and answering 200 with "deleted nothing" hides it. + var controller = BuildController(RepoWithRuns(), out _); + + var response = await controller.DeleteRuns(new AgentTestRunDeleteRequest()); + + Assert.IsType(response.Result); + } + + [Fact] + public async Task A_delete_of_nothing_but_blanks_is_rejected_too() + { + // Same bug wearing a different shape: a client that posts one empty row per unchecked box. + var controller = BuildController(RepoWithRuns(), out _); + + var response = await controller.DeleteRuns(new AgentTestRunDeleteRequest + { + RunIds = ["", " "] + }); + + Assert.IsType(response.Result); + } + + [Fact] + public void Clearing_run_history_requires_an_admin() + { + // Runs are the record of whether an agent change was evaluated at all, so removing them is at + // least as consequential as creating them -- the same gate triggering sits behind. + var method = typeof(AgentTestController).GetMethod(nameof(AgentTestController.DeleteRuns)); + + Assert.NotNull(method); + Assert.NotEmpty(method!.GetCustomAttributes(typeof(BotSharpAuthAttribute), inherit: true)); + } } diff --git a/tests/BotSharp.Core.UnitTests/AgentTesting/AgentTestRunExecutorTests.cs b/tests/BotSharp.Core.UnitTests/AgentTesting/AgentTestRunExecutorTests.cs index be3cf7434..b2527f583 100644 --- a/tests/BotSharp.Core.UnitTests/AgentTesting/AgentTestRunExecutorTests.cs +++ b/tests/BotSharp.Core.UnitTests/AgentTesting/AgentTestRunExecutorTests.cs @@ -43,6 +43,11 @@ private sealed class InMemoryRepo : IAgentTestRepository public Task> ListRunsByStatusAsync(string status) => Task.FromResult(Run.Status == status ? new List { Run } : []); public Task UpdateRunAsync(AgentTestRun run) { Run = run; return Task.CompletedTask; } + public Task DeleteRunAsync(string id) + { + var removed = Results.RemoveAll(r => r.RunId == id); + return Task.FromResult((long)removed); + } public Task AddCaseResultAsync(AgentTestCaseResult result) { Results.Add(result); return Task.CompletedTask; } public Task> ListCaseResultsAsync(string runId) => Task.FromResult(Results); } @@ -85,6 +90,11 @@ private sealed class CloningRunRepo : IAgentTestRepository public Task> ListRunsByStatusAsync(string status) => Task.FromResult(_stored.Status == status ? new List { Clone(_stored) } : []); public Task UpdateRunAsync(AgentTestRun run) { _stored = Clone(run); return Task.CompletedTask; } + public Task DeleteRunAsync(string id) + { + var removed = Results.RemoveAll(r => r.RunId == id); + return Task.FromResult((long)removed); + } public Task AddCaseResultAsync(AgentTestCaseResult result) { Results.Add(result); return Task.CompletedTask; } public Task> ListCaseResultsAsync(string runId) => Task.FromResult(Results); From ff0330f646d069c7332dab9ee9aed9a55a861183 Mon Sep 17 00:00:00 2001 From: "mars.yu" Date: Mon, 24 Aug 2026 16:10:52 +0800 Subject: [PATCH 3/3] Agent test harness: conversational case authoring (POST /agent-test/author) Lets a QA/PM write or edit a test case by chat instead of hand-writing turns, mocks and assertions. Stateless on the server -- the whole authoring conversation and the current draft travel with each request -- and it never saves: the draft still goes through the same create/update endpoints and the same CaseValidation those already ran, now shared instead of duplicated. Four guards on what a model may do to someone's draft: only fields it declares in changedFields are taken from its answer (an omitted field is kept, never deleted); a mock or toolCalled/toolNotCalled naming a function the agent cannot call is dropped rather than stored; a draft that fails validation gets one repair round against the real error text, and if that still fails the original draft comes back untouched; the change list shown to the user is diffed from the two drafts, never read off the model's own account. Also fixes a real failure: models routinely write argsMatchJson/resultContent/ a state value as a nested object instead of a JSON-string, which is valid JSON overall and so slipped past the existing "is this JSON" check and only broke at strong-typed deserialization. Normalises that shape deterministically before parsing, and separately gives a genuinely unparseable reply one retry (the validation-repair round already had one; this one didn't). Co-Authored-By: Claude Sonnet 5 --- .../AgentTestingPlugin.cs | 4 + .../Controllers/AgentTestController.cs | 222 ++-- .../Models/AgentTestAuthorDtos.cs | 159 +++ .../Models/AgentTestCase.cs | 2 +- .../Services/AssertionEvaluator.cs | 19 + .../Services/CaseValidation.cs | 146 +++ .../Services/ICaseAuthor.cs | 52 + .../Services/LlmCaseAuthor.cs | 1032 +++++++++++++++++ .../Services/MockTargetCatalogue.cs | 112 ++ .../AgentTesting/AgentTestControllerTests.cs | 6 +- .../AgentTesting/CaseAuthoringTests.cs | 522 +++++++++ 11 files changed, 2129 insertions(+), 147 deletions(-) create mode 100644 src/Plugins/BotSharp.Plugin.AgentTesting/Models/AgentTestAuthorDtos.cs create mode 100644 src/Plugins/BotSharp.Plugin.AgentTesting/Services/CaseValidation.cs create mode 100644 src/Plugins/BotSharp.Plugin.AgentTesting/Services/ICaseAuthor.cs create mode 100644 src/Plugins/BotSharp.Plugin.AgentTesting/Services/LlmCaseAuthor.cs create mode 100644 src/Plugins/BotSharp.Plugin.AgentTesting/Services/MockTargetCatalogue.cs create mode 100644 tests/BotSharp.Core.UnitTests/AgentTesting/CaseAuthoringTests.cs diff --git a/src/Plugins/BotSharp.Plugin.AgentTesting/AgentTestingPlugin.cs b/src/Plugins/BotSharp.Plugin.AgentTesting/AgentTestingPlugin.cs index 1e3bdd742..3695f85ba 100644 --- a/src/Plugins/BotSharp.Plugin.AgentTesting/AgentTestingPlugin.cs +++ b/src/Plugins/BotSharp.Plugin.AgentTesting/AgentTestingPlugin.cs @@ -87,6 +87,10 @@ public void RegisterDI(IServiceCollection services, IConfiguration config) // Scoped, like the segmenter: it resolves IChatCompletion implementations out of the same // scope and holds no state between calls. services.AddScoped(); + + // Scoped for the same reason, and reads the case store to ground its prompt in the suite's + // existing cases and the edited case's last run. + services.AddScoped(); services.AddScoped(); // AgentTestRunQueue is both a singleton and a BackgroundService: all three lines point at diff --git a/src/Plugins/BotSharp.Plugin.AgentTesting/Controllers/AgentTestController.cs b/src/Plugins/BotSharp.Plugin.AgentTesting/Controllers/AgentTestController.cs index 145e2d738..f507f545d 100644 --- a/src/Plugins/BotSharp.Plugin.AgentTesting/Controllers/AgentTestController.cs +++ b/src/Plugins/BotSharp.Plugin.AgentTesting/Controllers/AgentTestController.cs @@ -27,6 +27,7 @@ public class AgentTestController : ControllerBase private readonly IAgentTestRunQueue _queue; private readonly IAgentService _agents; private readonly AgentTestRecorder _recorder; + private readonly ICaseAuthor _author; private readonly ILlmProviderService _llmProviders; public AgentTestController( @@ -34,12 +35,14 @@ public AgentTestController( IAgentTestRunQueue queue, IAgentService agents, AgentTestRecorder recorder, + ICaseAuthor author, ILlmProviderService llmProviders) { _repo = repo; _queue = queue; _agents = agents; _recorder = recorder; + _author = author; _llmProviders = llmProviders; } @@ -134,7 +137,7 @@ public async Task> CreateCase([FromBody] AgentTestCa return BadRequest(entryAgentError); } - if (ValidateCasePayload(request) is { } validationError) + if (CaseValidation.Validate(request) is { } validationError) { return BadRequest(validationError); } @@ -178,7 +181,7 @@ public async Task> UpdateCase(string id, [FromBody] return BadRequest(entryAgentError); } - if (ValidateCasePayload(request) is { } validationError) + if (CaseValidation.Validate(request) is { } validationError) { return BadRequest(validationError); } @@ -480,6 +483,73 @@ public async Task>> RecordCase([FromBody] Agent } } + /// + /// One turn of authoring a case by conversation: the caller sends what they want in their own + /// words plus the draft as it stands, and gets back the new draft, a field-level diff, and + /// anything wrong with it. + /// + /// Saves nothing. The draft returned here still has to go through POST/PUT /agent-test/cases, + /// which is the only path that runs the entry-agent lookup and the only one a human presses -- + /// see for why an authoring model is not given write access to the + /// case store. + /// + /// [BotSharpAuth]: the same two escalations RecordCase and TriggerRun carry. It spends token + /// quota on every call with no throttling, and it sends the agent's instruction, the draft, and + /// (when the case has run before) the agent's real replies and real tool arguments to the model + /// vendor -- a wider egress than the recorder's, which withholds tool arguments. + /// + [BotSharpAuth] + [HttpPost("author")] + public async Task> AuthorCase([FromBody] AgentTestAuthorRequest request) + { + if (string.IsNullOrWhiteSpace(request.SuiteId)) + { + return BadRequest("suiteId is required"); + } + + // Same guard as RecordCase: a model this host cannot run fails here with a readable message, + // not deep inside the completion call. + if (ValidateRequestedModels(request.Model == null ? null : [request.Model]) is { } modelError) + { + return BadRequest(modelError); + } + + var suite = await _repo.GetSuiteAsync(request.SuiteId); + if (suite == null) + { + return NotFound($"agent test suite {request.SuiteId} not found"); + } + + if (!string.IsNullOrWhiteSpace(request.CaseId)) + { + // Checked rather than ignored: a caseId belonging to another suite would silently produce + // an ungrounded draft (the run-result lookup is scoped to this suite), and the author + // would never learn that the "based on the last run" part did nothing. + var existing = await _repo.GetCaseAsync(request.CaseId); + if (existing == null) + { + return NotFound($"agent test case {request.CaseId} not found"); + } + + if (!string.Equals(existing.SuiteId, suite.Id, StringComparison.Ordinal)) + { + return BadRequest($"case {request.CaseId} does not belong to suite {suite.Id}"); + } + } + + try + { + return await _author.AuthorAsync(suite, request, HttpContext.RequestAborted); + } + catch (CaseAuthorUnavailableException ex) + { + // No draft was produced. Surfaced verbatim, the same way RecordCase surfaces a rejected + // segmentation: "the model did not return JSON" tells the caller to retry, and "set this + // suite's judgeProvider" tells them what to fix. A 500 would tell them neither. + return BadRequest($"AI authoring failed: {ex.Message}"); + } + } + /// /// [BotSharpAuth]: every trigger really calls the model and really spends token quota, with no /// usage throttling anywhere -- a cost-escalation surface just like RecordCase, so it is @@ -599,16 +669,10 @@ public async Task>> GetMockTargets([FromQuery] string? return NotFound($"agent {agentId} not found"); } - var names = new List(); - names.AddRange((agent.Functions ?? []).Select(f => f.Name)); - names.AddRange((agent.SecondaryFunctions ?? []).Select(f => f.Name)); - names.AddRange((agent.McpTools ?? []).SelectMany(t => t.Functions ?? []).Select(f => f.Name)); - - return names - .Where(n => !string.IsNullOrWhiteSpace(n)) - .Distinct(StringComparer.OrdinalIgnoreCase) - .OrderBy(n => n, StringComparer.OrdinalIgnoreCase) - .ToList(); + // Same derivation the authoring prompt uses, so the names the editor offers and the names + // a model is allowed to mock can never diverge. Still names only on the wire: the case + // editor's pickers consume a plain string list. + return MockTargetCatalogue.Names(agent); } /// @@ -669,7 +733,7 @@ private static void ApplyCase(AgentTestCase testCase, AgentTestCaseUpsertRequest testCase.SuiteId = request.SuiteId; testCase.Name = request.Name; testCase.Enabled = request.Enabled; - // Normalised, not taken verbatim: ValidateCasePayload has already rejected anything that is + // Normalised, not taken verbatim: CaseValidation.Validate has already rejected anything that is // neither blank nor a known type, so this only fixes casing ("routing" -> "Routing") and // maps blank onto the default. Storing the caller's casing would break every later // Ordinal comparison against CaseTypes.Routing. @@ -712,130 +776,9 @@ private static void ApplyCase(AgentTestCase testCase, AgentTestCaseUpsertRequest testCase.LastReviewedDate = request.LastReviewedDate; } - /// - /// Shared create/update validation for a case payload. Null means the payload is acceptable; - /// otherwise the string is a caller-facing 400 message. - /// - private static string? ValidateCasePayload(AgentTestCaseUpsertRequest request) - { - if (IsUnsupportedUnmockedToolPolicy(request.UnmockedToolPolicy)) - { - return "Passthrough is not supported in P1"; - } - - var caseType = CaseTypes.Normalize(request.CaseType); - if (caseType == null && !string.IsNullOrWhiteSpace(request.CaseType)) - { - return $"caseType must be one of {string.Join(", ", CaseTypes.All)}, not '{request.CaseType}'"; - } - - if (CasePriorities.Normalize(request.Priority) == null && !string.IsNullOrWhiteSpace(request.Priority)) - { - return $"priority must be one of {string.Join(", ", CasePriorities.All)}, not '{request.Priority}'"; - } - - if (CaseSeverities.Normalize(request.Severity) == null && !string.IsNullOrWhiteSpace(request.Severity)) - { - return $"severity must be one of {string.Join(", ", CaseSeverities.All)}, not '{request.Severity}'"; - } - - // Rejected rather than clamped: a batch of 4 is a mistake, and silently filing the case in - // batch 3 would leave the author believing it runs somewhere it does not. - if (request.Batch is { } batch && !CaseBatches.All.Contains(batch)) - { - return $"batch must be one of {string.Join(", ", CaseBatches.All)}, not {batch}"; - } - - foreach (var (message, index) in (request.History ?? []).Select((m, i) => (m, i))) - { - if (HistoryRoles.Normalize(message?.Role) == null) - { - return $"history message {index + 1} has role '{message?.Role}'; only " - + $"{string.Join(" and ", HistoryRoles.All)} are supported"; - } - - // An empty message is dropped by BotSharp's own dialog storage (ConversationStorage - // skips elements with blank content), so it would silently not be there at run time -- - // and the runner's count check would then fail the whole case with a confusing message - // about the write having vanished. - if (string.IsNullOrWhiteSpace(message!.Content)) - { - return $"history message {index + 1} has no content"; - } - } - - var allAssertions = (request.Turns ?? []) - .SelectMany(t => t.Assertions ?? []) - .Concat(request.Assertions ?? []) - .ToList(); - - foreach (var assertion in allAssertions) - { - var error = AssertionValidation.Validate(assertion); - if (error != null) - { - return error; - } - } - - return ValidateRoutingCase(caseType ?? CaseTypes.Agent, request, allAssertions); - } - - /// - /// The extra rules a Routing case has to satisfy. A Routing case is not a label -- it is the only - /// type counted towards a run's routing accuracy, so one that cannot actually establish a routing - /// outcome would quietly move that figure without measuring anything. - /// - /// Enforced at save time for the same reason AssertionValidation is: the alternative is a case - /// that saves cleanly and then reports a meaningless green every run. - /// - private static string? ValidateRoutingCase( - string caseType, AgentTestCaseUpsertRequest request, List allAssertions) - { - if (!string.Equals(caseType, CaseTypes.Routing, StringComparison.Ordinal)) - { - return null; - } - - // Routing is a single-turn question: which agent picks this message up. A second turn is - // either a different question (making it an Agent case) or an accident, and either way its - // result would be counted as routing accuracy. - // - // Authored History is not a turn and is deliberately not counted here: replaying a prior - // exchange and then asking one question is still a single routing decision, and it is the - // most realistic way to test routing that depends on context. - if ((request.Turns ?? []).Count != 1) - { - return "a Routing case must have exactly one turn; use an Agent case for a multi-turn case"; - } - - // Without one of these the case asserts nothing about routing, yet still counts towards - // routing accuracy -- it would report Passed for having successfully said anything at all. - var assertsRouting = allAssertions.Any(a => - string.Equals(a.Type, AssertionTypes.RoutedToAgent, StringComparison.Ordinal) - || string.Equals(a.Type, AssertionTypes.AgentChain, StringComparison.Ordinal)); - if (!assertsRouting) - { - return $"a Routing case needs at least one '{AssertionTypes.RoutedToAgent}' or " - + $"'{AssertionTypes.AgentChain}' assertion, otherwise it verifies no routing outcome"; - } - - // The framework this implements scores routing purely as expected-agent == actual-agent and - // deliberately applies no quality judgement to it. An llmJudge here would also make the - // routing figure depend on a vendor call, so a vendor outage would read as a routing - // regression. - if (allAssertions.Any(a => string.Equals(a.Type, AssertionTypes.LlmJudge, StringComparison.Ordinal))) - { - return $"a Routing case cannot use '{AssertionTypes.LlmJudge}': routing is judged only by " - + "which agent handled the conversation"; - } - - return null; - } - /// /// Null when the request's entry agent is usable. Separate from - /// because it needs IAgentService, so it cannot be static, and + /// because it needs IAgentService, so it cannot be static, and /// a lookup is worth doing only once the cheap checks have passed. /// private async Task ValidateEntryAgentAsync(AgentTestCaseUpsertRequest request) @@ -849,15 +792,6 @@ private static void ApplyCase(AgentTestCase testCase, AgentTestCaseUpsertRequest return agent == null ? $"entry agent {request.EntryAgentId} not found" : null; } - /// - /// Passthrough was specified in the design/plan and even had a (dead) code path, but nothing - /// ever back-fills an ObservedToolCall for a tool the provider let run for real -- under it, - /// toolNotCalled always vacuously passed against a tool that genuinely executed with real side - /// effects. Rejected here rather than implementing the back-fill (project owner decision). - /// - private static bool IsUnsupportedUnmockedToolPolicy(string? policy) - => string.Equals(policy, "Passthrough", StringComparison.OrdinalIgnoreCase); - private static bool IsTerminalStatus(string status) => status is AgentTestStatus.Passed or AgentTestStatus.Failed or AgentTestStatus.Error or AgentTestStatus.Cancelled; diff --git a/src/Plugins/BotSharp.Plugin.AgentTesting/Models/AgentTestAuthorDtos.cs b/src/Plugins/BotSharp.Plugin.AgentTesting/Models/AgentTestAuthorDtos.cs new file mode 100644 index 000000000..8f636fbb2 --- /dev/null +++ b/src/Plugins/BotSharp.Plugin.AgentTesting/Models/AgentTestAuthorDtos.cs @@ -0,0 +1,159 @@ +namespace BotSharp.Plugin.AgentTesting.Models; + +/// +/// Body of POST /agent-test/author -- one turn of authoring a case by conversation. +/// +/// Deliberately carries the whole conversation and the whole current draft, because the server keeps +/// no authoring session: there is no fifth collection, no draft lifecycle, no lock, and nothing to +/// clean up when someone closes the tab mid-sentence. The editor page already holds the draft as its +/// form state, so the draft it sends is exactly what the user is looking at, and what comes back +/// replaces it. +/// +public class AgentTestAuthorRequest +{ + /// Required: supplies the agent under test, and the judge model this falls back to. + public string SuiteId { get; set; } = string.Empty; + + /// + /// The case being edited, when there is one. Null while creating. + /// + /// Used only to ground the model: with a case id its most recent run result can be read, which + /// is what turns "add an assertion" from invention into a proposal based on what the agent + /// actually said and actually passed to its tools. + /// + public string? CaseId { get; set; } + + /// The authoring conversation so far, oldest first. The last entry is the new instruction. + public List Messages { get; set; } = []; + + /// + /// The draft as it stands. Null means start from nothing, which is the first turn of creating a + /// case. + /// + public AgentTestCaseUpsertRequest? Draft { get; set; } + + /// + /// Optional model override. Null falls back to the suite's judge model, and if that is unset the + /// request is refused rather than defaulting to a model nobody chose -- the same stance + /// LlmAgentTestJudge takes. + /// + public TestModel? Model { get; set; } +} + +/// One message in the authoring conversation. Only user and assistant: see . +public class AuthorChatMessage +{ + public string Role { get; set; } = AuthorChatRoles.User; + public string Content { get; set; } = string.Empty; +} + +public static class AuthorChatRoles +{ + public const string User = "user"; + public const string Assistant = "assistant"; + + public static readonly string[] All = [User, Assistant]; + + public static string? Normalize(string? value) + => All.FirstOrDefault(r => string.Equals(r, value?.Trim(), StringComparison.OrdinalIgnoreCase)); +} + +/// +/// Result of one authoring turn: what to say to the user, and what the draft now is. +/// +/// The draft is never saved by this endpoint. Persistence stays on POST/PUT /agent-test/cases, which +/// is the only path that runs the full validation including the entry-agent lookup -- and which the +/// human presses deliberately. An authoring model that could write to the case store would be one +/// misread instruction away from editing a case nobody asked it to touch. +/// +public class AgentTestAuthorResponse +{ + /// What the model says about what it did, shown in the chat panel. + public string Reply { get; set; } = string.Empty; + + /// + /// The draft after the merge. Always populated, even when nothing changed, so the client can + /// assign it unconditionally. + /// + public AgentTestCaseUpsertRequest Draft { get; set; } = new(); + + /// True when differs from the one that was sent in. + public bool DraftChanged { get; set; } + + /// + /// Field-level diff, computed by comparing the incoming draft with the merged one -- never taken + /// from the model's own account of what it did. A model that says it added one assertion while + /// actually rewriting every turn has to be caught by something that does not ask it. + /// + public List Changes { get; set; } = []; + + /// + /// What says about the merged + /// draft. Non-empty means the draft cannot be saved as it stands, and the client shows it as + /// such rather than letting the user discover it on the save button. + /// + public List ValidationErrors { get; set; } = []; + + /// + /// Things that were silently wrong and got corrected, or are suspect and were left alone: a mock + /// dropped for naming a function this agent cannot call, a state key nothing else in the suite + /// has ever used, an llmJudge assertion on a suite with no judge model. + /// + public List Warnings { get; set; } = []; +} + +/// One changed field of a draft. +public class AuthorChange +{ + /// The draft field name, as the client knows it (camelCase, e.g. "turns"). + public string Field { get; set; } = string.Empty; + + /// Short human summary of the change, e.g. "3 turns -> 4 turns". + public string Detail { get; set; } = string.Empty; +} + +/// +/// The draft fields an authoring model is allowed to change. +/// +/// A whitelist, and the merge copies everything else straight off the incoming draft, so a model +/// that omits a field from its answer cannot delete it -- the failure mode of returning a whole +/// document is silent data loss, and this is what removes it. The visible cost is that a model which +/// forgets to declare a field it meant to change makes no change at all, which the user sees and can +/// simply ask for again. +/// +/// Four writable case fields are deliberately absent: +/// suiteId -- comes from the request, not from a model; +/// unmockedToolPolicy -- only Block is supported, and a model proposing Passthrough is exactly the +/// thing validation exists to stop; +/// sourceConversationId -- a provenance record, not authoring; +/// lastReviewedDate -- a human attestation that the case still reflects reality, which is worth +/// nothing if a model can stamp it. +/// +public static class AuthorFields +{ + public const string Name = "name"; + public const string Enabled = "enabled"; + public const string CaseType = "caseType"; + public const string EntryAgentId = "entryAgentId"; + public const string Turns = "turns"; + public const string Assertions = "assertions"; + public const string InitialStates = "initialStates"; + public const string History = "history"; + public const string Mocks = "mocks"; + public const string Priority = "priority"; + public const string Severity = "severity"; + public const string Batch = "batch"; + public const string CrossCutting = "crossCutting"; + public const string InvolvedAgents = "involvedAgents"; + public const string BusinessDomain = "businessDomain"; + public const string ExpectedOutcome = "expectedOutcome"; + + public static readonly string[] All = + [ + Name, Enabled, CaseType, EntryAgentId, Turns, Assertions, InitialStates, History, Mocks, + Priority, Severity, Batch, CrossCutting, InvolvedAgents, BusinessDomain, ExpectedOutcome + ]; + + public static string? Normalize(string? value) + => All.FirstOrDefault(f => string.Equals(f, value?.Trim(), StringComparison.OrdinalIgnoreCase)); +} diff --git a/src/Plugins/BotSharp.Plugin.AgentTesting/Models/AgentTestCase.cs b/src/Plugins/BotSharp.Plugin.AgentTesting/Models/AgentTestCase.cs index 11394540d..7cfabd6da 100644 --- a/src/Plugins/BotSharp.Plugin.AgentTesting/Models/AgentTestCase.cs +++ b/src/Plugins/BotSharp.Plugin.AgentTesting/Models/AgentTestCase.cs @@ -14,7 +14,7 @@ public class AgentTestCase : MongoBase /// initialiser, so old documents read back as Agent cases rather than as an invalid blank. /// /// Not cosmetic: a Routing case is validated differently (single turn, must actually assert a - /// routing outcome, no llmJudge -- see AgentTestController.ValidateCasePayload) and is the only + /// routing outcome, no llmJudge -- see CaseValidation.Validate) and is the only /// type counted towards a run's routing accuracy. /// public string CaseType { get; set; } = CaseTypes.Agent; diff --git a/src/Plugins/BotSharp.Plugin.AgentTesting/Services/AssertionEvaluator.cs b/src/Plugins/BotSharp.Plugin.AgentTesting/Services/AssertionEvaluator.cs index 9c3554eea..ac783dc23 100644 --- a/src/Plugins/BotSharp.Plugin.AgentTesting/Services/AssertionEvaluator.cs +++ b/src/Plugins/BotSharp.Plugin.AgentTesting/Services/AssertionEvaluator.cs @@ -365,6 +365,25 @@ private enum RequiredField { Expected, Target } [AssertionTypes.LlmJudge] = RequiredField.Expected, }; + /// + /// The nine authorable types, in the order the Requirements map above declares them. + /// + /// Exposed so that ICaseAuthor can generate the assertion vocabulary it puts in front of a model + /// from this map rather than from a hand-written copy in a prompt string. A prompt that lists a + /// type this map does not know, or omits one it does, produces drafts that fail validation for + /// reasons the author cannot see. + /// + public static IReadOnlyList Authorable { get; } = Requirements.Keys.ToArray(); + + /// + /// Which field this assertion type must carry: "expected", "target", or null for a type with no + /// such requirement. + /// + public static string? RequiredFieldName(string type) + => Requirements.TryGetValue(type, out var required) + ? required == RequiredField.Expected ? "expected" : "target" + : null; + /// Null when the assertion is well-formed; otherwise a caller-facing error message. public static string? Validate(TestAssertion assertion) { diff --git a/src/Plugins/BotSharp.Plugin.AgentTesting/Services/CaseValidation.cs b/src/Plugins/BotSharp.Plugin.AgentTesting/Services/CaseValidation.cs new file mode 100644 index 000000000..6a413e44b --- /dev/null +++ b/src/Plugins/BotSharp.Plugin.AgentTesting/Services/CaseValidation.cs @@ -0,0 +1,146 @@ +namespace BotSharp.Plugin.AgentTesting.Services; + +/// +/// Shared create/update validation for a case payload, lifted out of AgentTestController so that +/// every path which produces a case runs the same rules. +/// +/// That sharing is the point, not tidiness: hands a model-authored draft +/// back to a human to save, and a draft that only fails when they press Save wastes the round trip +/// and teaches them to distrust the feature. The author service runs these rules itself and gives +/// the model one chance to repair its own output against the real error text. +/// +/// Everything here is static and I/O-free. The one case rule that needs a service -- +/// does the entry agent exist -- stays on the controller as ValidateEntryAgentAsync. +/// +public static class CaseValidation +{ + /// + /// Shared create/update validation for a case payload. Null means the payload is acceptable; + /// otherwise the string is a caller-facing 400 message. + /// + public static string? Validate(AgentTestCaseUpsertRequest request) + { + if (IsUnsupportedUnmockedToolPolicy(request.UnmockedToolPolicy)) + { + return "Passthrough is not supported in P1"; + } + + var caseType = CaseTypes.Normalize(request.CaseType); + if (caseType == null && !string.IsNullOrWhiteSpace(request.CaseType)) + { + return $"caseType must be one of {string.Join(", ", CaseTypes.All)}, not '{request.CaseType}'"; + } + + if (CasePriorities.Normalize(request.Priority) == null && !string.IsNullOrWhiteSpace(request.Priority)) + { + return $"priority must be one of {string.Join(", ", CasePriorities.All)}, not '{request.Priority}'"; + } + + if (CaseSeverities.Normalize(request.Severity) == null && !string.IsNullOrWhiteSpace(request.Severity)) + { + return $"severity must be one of {string.Join(", ", CaseSeverities.All)}, not '{request.Severity}'"; + } + + // Rejected rather than clamped: a batch of 4 is a mistake, and silently filing the case in + // batch 3 would leave the author believing it runs somewhere it does not. + if (request.Batch is { } batch && !CaseBatches.All.Contains(batch)) + { + return $"batch must be one of {string.Join(", ", CaseBatches.All)}, not {batch}"; + } + + foreach (var (message, index) in (request.History ?? []).Select((m, i) => (m, i))) + { + if (HistoryRoles.Normalize(message?.Role) == null) + { + return $"history message {index + 1} has role '{message?.Role}'; only " + + $"{string.Join(" and ", HistoryRoles.All)} are supported"; + } + + // An empty message is dropped by BotSharp's own dialog storage (ConversationStorage + // skips elements with blank content), so it would silently not be there at run time -- + // and the runner's count check would then fail the whole case with a confusing message + // about the write having vanished. + if (string.IsNullOrWhiteSpace(message!.Content)) + { + return $"history message {index + 1} has no content"; + } + } + + var allAssertions = (request.Turns ?? []) + .SelectMany(t => t.Assertions ?? []) + .Concat(request.Assertions ?? []) + .ToList(); + + foreach (var assertion in allAssertions) + { + var error = AssertionValidation.Validate(assertion); + if (error != null) + { + return error; + } + } + + return ValidateRoutingCase(caseType ?? CaseTypes.Agent, request, allAssertions); + } + + /// + /// The extra rules a Routing case has to satisfy. A Routing case is not a label -- it is the only + /// type counted towards a run's routing accuracy, so one that cannot actually establish a routing + /// outcome would quietly move that figure without measuring anything. + /// + /// Enforced at save time for the same reason AssertionValidation is: the alternative is a case + /// that saves cleanly and then reports a meaningless green every run. + /// + private static string? ValidateRoutingCase( + string caseType, AgentTestCaseUpsertRequest request, List allAssertions) + { + if (!string.Equals(caseType, CaseTypes.Routing, StringComparison.Ordinal)) + { + return null; + } + + // Routing is a single-turn question: which agent picks this message up. A second turn is + // either a different question (making it an Agent case) or an accident, and either way its + // result would be counted as routing accuracy. + // + // Authored History is not a turn and is deliberately not counted here: replaying a prior + // exchange and then asking one question is still a single routing decision, and it is the + // most realistic way to test routing that depends on context. + if ((request.Turns ?? []).Count != 1) + { + return "a Routing case must have exactly one turn; use an Agent case for a multi-turn case"; + } + + // Without one of these the case asserts nothing about routing, yet still counts towards + // routing accuracy -- it would report Passed for having successfully said anything at all. + var assertsRouting = allAssertions.Any(a => + string.Equals(a.Type, AssertionTypes.RoutedToAgent, StringComparison.Ordinal) + || string.Equals(a.Type, AssertionTypes.AgentChain, StringComparison.Ordinal)); + if (!assertsRouting) + { + return $"a Routing case needs at least one '{AssertionTypes.RoutedToAgent}' or " + + $"'{AssertionTypes.AgentChain}' assertion, otherwise it verifies no routing outcome"; + } + + // The framework this implements scores routing purely as expected-agent == actual-agent and + // deliberately applies no quality judgement to it. An llmJudge here would also make the + // routing figure depend on a vendor call, so a vendor outage would read as a routing + // regression. + if (allAssertions.Any(a => string.Equals(a.Type, AssertionTypes.LlmJudge, StringComparison.Ordinal))) + { + return $"a Routing case cannot use '{AssertionTypes.LlmJudge}': routing is judged only by " + + "which agent handled the conversation"; + } + + return null; + } + + /// + /// Passthrough was specified in the design/plan and even had a (dead) code path, but nothing + /// ever back-fills an ObservedToolCall for a tool the provider let run for real -- under it, + /// toolNotCalled always vacuously passed against a tool that genuinely executed with real side + /// effects. Rejected here rather than implementing the back-fill (project owner decision). + /// + private static bool IsUnsupportedUnmockedToolPolicy(string? policy) + => string.Equals(policy, "Passthrough", StringComparison.OrdinalIgnoreCase); +} diff --git a/src/Plugins/BotSharp.Plugin.AgentTesting/Services/ICaseAuthor.cs b/src/Plugins/BotSharp.Plugin.AgentTesting/Services/ICaseAuthor.cs new file mode 100644 index 000000000..d65105ac8 --- /dev/null +++ b/src/Plugins/BotSharp.Plugin.AgentTesting/Services/ICaseAuthor.cs @@ -0,0 +1,52 @@ +namespace BotSharp.Plugin.AgentTesting.Services; + +/// +/// Authors and edits a test case by conversation: the user picks an agent's suite, says what they +/// want in their own words, and gets back a draft they can keep talking to. +/// +/// Contract: +/// - Produces a draft, never a stored case. Nothing here writes to the case store; the response goes +/// back to the editor, and the human presses save. See . +/// - Never deletes by omission. The model declares which fields it is changing and only those are +/// taken from its answer; everything else is copied off the draft that came in. See +/// . +/// - Never invents a callable function. A mock or a toolCalled assertion naming a function the agent +/// cannot call is dropped and reported, because it could otherwise never match at run time. +/// - Runs on its own output and gives the model one chance to repair a +/// rejection against the real error text. A draft that still fails is returned as the unchanged +/// original plus the errors -- never as a broken draft presented as progress. +/// +/// Boundary -- what leaves this system. The agent's own instruction, its function names and +/// descriptions, the suite's existing case names, the draft, and the user's authoring messages are +/// all sent to the configured model vendor. When the case has run before, so are the agent's actual +/// reply texts and its actual tool arguments from that run, which for a case recorded from a real +/// conversation can include the customer data that conversation contained. That is a wider egress +/// than the recorder's (which withholds tool arguments and results), and it is why the endpoint is +/// admin-only. +/// +public interface ICaseAuthor +{ + /// + /// One authoring turn. + /// + /// + /// No usable model, the vendor call failed, or the model's answer could not be read as an + /// authoring result. All of these mean "no draft was produced", which is a different thing from + /// "the draft is invalid" -- the latter comes back in the response. + /// + Task AuthorAsync( + AgentTestSuite suite, + AgentTestAuthorRequest request, + CancellationToken ct); +} + +/// +/// No draft could be produced. Separate from a validation failure on purpose: a validation failure +/// still returns a draft and a next step, while this means the authoring turn did not happen and the +/// user should retry or fix configuration. +/// +public class CaseAuthorUnavailableException : Exception +{ + public CaseAuthorUnavailableException(string message) : base(message) { } + public CaseAuthorUnavailableException(string message, Exception inner) : base(message, inner) { } +} diff --git a/src/Plugins/BotSharp.Plugin.AgentTesting/Services/LlmCaseAuthor.cs b/src/Plugins/BotSharp.Plugin.AgentTesting/Services/LlmCaseAuthor.cs new file mode 100644 index 000000000..e023ef878 --- /dev/null +++ b/src/Plugins/BotSharp.Plugin.AgentTesting/Services/LlmCaseAuthor.cs @@ -0,0 +1,1032 @@ +using System.Text; +using System.Text.Json; +using System.Text.Json.Nodes; +using System.Text.Json.Serialization; +using BotSharp.Abstraction.Agents; +using BotSharp.Abstraction.Agents.Enums; +using BotSharp.Abstraction.MLTasks; +using BotSharp.Plugin.AgentTesting.Repositories; + +namespace BotSharp.Plugin.AgentTesting.Services; + +/// +/// The model-backed . Contract and boundary live on the interface. +/// +/// Extends the model's answer no trust, in four separate places, because each guards a different +/// failure: the whitelist merge stops a field being deleted by omission, the function-name check +/// stops a mock that could never match, the validation pass stops a draft that cannot be saved, and +/// the diff is computed rather than read off the model's own account of what it did. +/// +public class LlmCaseAuthor : ICaseAuthor +{ + /// + /// How much of the authoring conversation is replayed. The draft carries the state, so older + /// turns add tokens without adding information -- and every turn of this chat pays for the whole + /// context block again. + /// + private const int MaxChatMessages = 20; + + private const int MaxInstructionChars = 4000; + private const int MaxExistingCases = 25; + private const int MaxGroundedTurns = 10; + private const int MaxGroundedOutputChars = 600; + private const int MaxGroundedToolCalls = 20; + private const int MaxGroundedArgsChars = 300; + + /// + /// How many recent runs are searched for a result belonging to the case being edited. Bounded + /// because each one is a separate query, and a case that has not run in the last five runs of + /// its own suite is one whose old output would be misleading grounding anyway. + /// + private const int MaxRunsScanned = 5; + + /// + /// camelCase both ways: this is the shape the case editor already posts to /agent-test/cases, so + /// a draft can travel from the model straight into the editor's form and back out to the save + /// endpoint without a second naming convention in between. + /// + private static readonly JsonSerializerOptions DraftJson = new() + { + PropertyNamingPolicy = JsonNamingPolicy.CamelCase, + PropertyNameCaseInsensitive = true, + DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull + }; + + private readonly IServiceProvider _services; + private readonly IAgentService _agents; + private readonly IAgentTestRepository _repo; + private readonly ILogger _logger; + + public LlmCaseAuthor( + IServiceProvider services, + IAgentService agents, + IAgentTestRepository repo, + ILogger logger) + { + _services = services; + _agents = agents; + _repo = repo; + _logger = logger; + } + + public async Task AuthorAsync( + AgentTestSuite suite, + AgentTestAuthorRequest request, + CancellationToken ct) + { + var messages = (request.Messages ?? []) + .Where(m => !string.IsNullOrWhiteSpace(m?.Content)) + .ToList(); + + if (messages.Count == 0 + || !string.Equals(AuthorChatRoles.Normalize(messages[^1].Role), AuthorChatRoles.User, StringComparison.Ordinal)) + { + throw new CaseAuthorUnavailableException( + "the last message must be a user message saying what to do"); + } + + var model = ResolveModel(suite, request.Model); + var completion = _services.GetServices() + .FirstOrDefault(x => string.Equals(x.Provider, model.Provider, StringComparison.OrdinalIgnoreCase)) + ?? throw new CaseAuthorUnavailableException( + $"no chat completion provider is registered for '{model.Provider}'"); + + completion.SetModelName(model.Model); + + // The draft's own entry agent wins over the suite's, for the same reason the runner honours + // it: a case pointed at a leaf agent is testing that agent, and authoring it against the + // router's instruction and the router's functions would describe the wrong thing. + var agentId = string.IsNullOrWhiteSpace(request.Draft?.EntryAgentId) + ? suite.AgentId + : request.Draft!.EntryAgentId!.Trim(); + + var agent = await _agents.GetAgent(agentId) + ?? throw new CaseAuthorUnavailableException( + $"agent {agentId} not found, so there is nothing to author a case against"); + + var targets = MockTargetCatalogue.Describe(agent); + var existingCases = await _repo.ListCasesAsync(suite.Id); + var grounding = await LoadGroundingAsync(suite.Id, request.CaseId, ct); + var agentNames = await AgentNamesAsync(); + + // The baseline is what every merge and every diff is measured against. Cloned so a merge + // cannot mutate the caller's object and leave the diff comparing a thing with itself. + var baseline = Clone(request.Draft ?? new AgentTestCaseUpsertRequest()); + baseline.SuiteId = suite.Id; + + var instruction = BuildInstruction(); + var dialogs = new List + { + new(AgentRole.User, BuildContext(agent, targets, existingCases, request.CaseId, grounding, agentNames)) + }; + + foreach (var message in messages.Take(messages.Count - 1).TakeLast(MaxChatMessages)) + { + var role = string.Equals(AuthorChatRoles.Normalize(message.Role), AuthorChatRoles.Assistant, StringComparison.Ordinal) + ? AgentRole.Assistant + : AgentRole.User; + + dialogs.Add(new RoleDialogModel(role, message.Content)); + } + + // The draft goes in the last message rather than the context block: it is the thing that + // changes every turn, and it is what the instruction tells the model to echo field names from. + dialogs.Add(new RoleDialogModel(AgentRole.User, BuildTask(baseline, messages[^1].Content))); + + var attempt = await AskWithParseRepairAsync(completion, instruction, dialogs, model, ct); + var response = Assemble(baseline, attempt, targets, existingCases, suite); + + if (response.ValidationErrors.Count == 0) + { + _logger.LogInformation( + "Case author changed {ChangeCount} field(s) with {Model}, {WarningCount} warning(s).", + response.Changes.Count, model, response.Warnings.Count); + + return response; + } + + // One repair round against the real error text -- the same single-retry stance the segmenter + // and the judge take. Merged from the baseline again, never from the rejected draft: + // repairing on top of something already invalid compounds the mistake. + _logger.LogInformation( + "Case author draft was rejected ({Error}); asking {Model} to repair it once.", + response.ValidationErrors[0], model); + + dialogs.Add(new RoleDialogModel(AgentRole.Assistant, attempt.Raw)); + dialogs.Add(new RoleDialogModel(AgentRole.User, + $""" + That draft was rejected by validation: + {string.Join("\n", response.ValidationErrors)} + + Fix exactly that and answer with the same JSON envelope again. + """)); + + var repaired = await AskWithParseRepairAsync(completion, instruction, dialogs, model, ct); + var second = Assemble(baseline, repaired, targets, existingCases, suite); + + if (second.ValidationErrors.Count == 0) + { + return second; + } + + // Still invalid. The baseline comes back untouched and the errors are stated: an invalid + // draft presented as progress would overwrite a working one in the editor. + _logger.LogWarning( + "Case author could not produce a valid draft after a repair round: {Error}", + second.ValidationErrors[0]); + + return new AgentTestAuthorResponse + { + Reply = second.Reply, + Draft = baseline, + DraftChanged = false, + Changes = [], + ValidationErrors = second.ValidationErrors, + Warnings = second.Warnings + }; + } + + /// + /// No silent default, for the reason LlmAgentTestJudge gives: BotSharp's own InstructService falls + /// back to openai/gpt-4o, and inheriting that here would author cases with a model nobody chose. + /// The suite's judge model is used as the fallback rather than a new setting, because a suite that + /// has one has already had a model chosen for it deliberately. + /// + private static TestModel ResolveModel(AgentTestSuite suite, TestModel? requested) + { + if (!string.IsNullOrWhiteSpace(requested?.Provider) && !string.IsNullOrWhiteSpace(requested?.Model)) + { + return requested!; + } + + if (!string.IsNullOrWhiteSpace(suite.JudgeProvider) && !string.IsNullOrWhiteSpace(suite.JudgeModel)) + { + return new TestModel { Provider = suite.JudgeProvider!, Model = suite.JudgeModel! }; + } + + throw new CaseAuthorUnavailableException( + "no model to author with: pass a provider and model, or set this suite's judgeProvider " + + "and judgeModel"); + } + + /// + /// Agent names, because routedToAgent and agentChain match by name or id -- and a model writing a + /// name it invented produces an assertion that can never pass, which reads as a routing + /// regression forever after. + /// + private async Task> AgentNamesAsync() + { + var options = await _agents.GetAgentOptions(); + + return (options ?? []) + .Select(o => o.Name) + .Where(n => !string.IsNullOrWhiteSpace(n)) + .Distinct(StringComparer.OrdinalIgnoreCase) + .OrderBy(n => n, StringComparer.OrdinalIgnoreCase) + .ToList(); + } + + /// + /// The most recent result for the case being edited, if it has ever run. This is what lets the + /// model propose an outputContains against text the agent really produced and an argsMatchJson + /// against arguments it really passed, instead of inventing both. + /// + private async Task LoadGroundingAsync(string suiteId, string? caseId, CancellationToken ct) + { + if (string.IsNullOrWhiteSpace(caseId)) + { + return null; + } + + var runs = await _repo.ListRunsAsync(suiteId); // newest first + foreach (var run in runs.Take(MaxRunsScanned)) + { + ct.ThrowIfCancellationRequested(); + + var results = await _repo.ListCaseResultsAsync(run.Id); + var hit = results.FirstOrDefault(r => string.Equals(r.CaseId, caseId, StringComparison.Ordinal)); + if (hit != null) + { + return hit; + } + } + + return null; + } + + /// + /// Calls the model and parses its reply, retrying once if the reply cannot be read at all -- + /// the same single-retry stance the validation-repair round in takes, + /// extended to cover the other way a model's answer can be unusable. already + /// repairs the commonest cause (a JSON-as-text field written as a nested object) deterministically + /// before this is ever reached, so landing here needs a genuinely different mistake -- truncated + /// output, a stray comma, prose with no JSON at all. + /// + private async Task AskWithParseRepairAsync( + IChatCompletion completion, + string instruction, + List dialogs, + TestModel model, + CancellationToken ct) + { + var (raw, attempt, error) = await AskOnceAsync(completion, instruction, dialogs, ct); + if (attempt != null) + { + return attempt; + } + + _logger.LogInformation( + "Case author reply could not be read ({Error}); asking {Model} to resend it.", error, model); + + dialogs.Add(new RoleDialogModel(AgentRole.Assistant, raw)); + dialogs.Add(new RoleDialogModel(AgentRole.User, + $""" + That reply could not be read: {error} + + Resend the full JSON envelope, valid this time. Remember: argsMatchJson, resultContent and + any state "value" must be a JSON string (escaped), never a nested object or array. + """)); + + var (_, repaired, repairError) = await AskOnceAsync(completion, instruction, dialogs, ct); + return repaired ?? throw new CaseAuthorUnavailableException( + $"the authoring model did not return a usable reply after a retry: {repairError}"); + } + + /// + /// One model call, parsed but never throwing on a parse failure -- the caller decides whether + /// that is retryable. A genuine vendor failure (timeout, rate limit, bad key) still throws + /// immediately: asking the same vendor again in the same turn cannot fix that, and treating it as + /// retryable would waste a call and hide the real error behind a generic "no usable reply". + /// + private async Task<(string Raw, AuthorAttempt? Attempt, string? Error)> AskOnceAsync( + IChatCompletion completion, + string instruction, + List dialogs, + CancellationToken ct) + { + ct.ThrowIfCancellationRequested(); + + var promptAgent = new Agent + { + Id = Guid.Empty.ToString(), + Name = "AgentTestCaseAuthor", + Instruction = instruction + }; + + string raw; + try + { + var response = await completion.GetChatCompletions(promptAgent, dialogs); + raw = response?.Content ?? string.Empty; + } + catch (Exception ex) + { + // A vendor timeout, a rate limit, a bad key. None of these produced a draft, and none of + // them should look like one. + throw new CaseAuthorUnavailableException($"the authoring model call failed: {ex.Message}", ex); + } + + try + { + return (raw, Parse(raw), null); + } + catch (CaseAuthorUnavailableException ex) + { + return (raw, null, ex.Message); + } + } + + /// + /// Reads the envelope. Public and static so the parsing rules can be tested without a vendor. + /// + public static AuthorAttempt Parse(string raw) + { + var json = ExtractJson(raw) + ?? throw new CaseAuthorUnavailableException( + $"the authoring model did not return JSON. First 200 chars: {Truncate(raw, 200)}"); + + json = CoerceStructuredJsonStringFields(json); + + AuthorEnvelope? envelope; + try + { + envelope = JsonSerializer.Deserialize(json, DraftJson); + } + catch (JsonException ex) + { + throw new CaseAuthorUnavailableException($"the authoring model returned malformed JSON: {ex.Message}"); + } + + if (envelope == null) + { + throw new CaseAuthorUnavailableException("the authoring model returned an empty result"); + } + + var declared = (envelope.ChangedFields ?? []) + .Select(AuthorFields.Normalize) + .Where(f => f != null) + .Select(f => f!) + .Distinct(StringComparer.Ordinal) + .ToList(); + + // An unwritable field name does not fail the turn -- the merge would ignore it anyway -- but + // it is reported, because "I renamed the suite for you" needs to be visibly untrue. + var rejected = (envelope.ChangedFields ?? []) + .Where(f => !string.IsNullOrWhiteSpace(f) && AuthorFields.Normalize(f) == null) + .Distinct(StringComparer.OrdinalIgnoreCase) + .ToList(); + + return new AuthorAttempt(envelope.Reply ?? string.Empty, declared, rejected, envelope.Draft, raw); + } + + /// + /// Merge, sanitise, validate, diff. Deterministic given the same attempt, catalogue and suite. + /// + /// Public and static for the reason and LlmAgentTestJudge.ParseVerdict are: + /// everything that decides what a model is allowed to do to a draft happens here, and it should + /// be testable without a vendor call. + /// + public static AgentTestAuthorResponse Assemble( + AgentTestCaseUpsertRequest baseline, + AuthorAttempt attempt, + List targets, + List existingCases, + AgentTestSuite suite) + { + var warnings = new List(); + + foreach (var field in attempt.RejectedFields) + { + warnings.Add($"the model asked to change '{field}', which is not a field it may change; ignored"); + } + + var merged = Clone(baseline); + + if (attempt.DeclaredFields.Count > 0) + { + if (attempt.Draft == null) + { + warnings.Add("the model said it changed the draft but returned no draft; nothing was changed"); + } + else + { + foreach (var field in attempt.DeclaredFields) + { + Apply(merged, attempt.Draft, field); + } + } + } + + // Never from the model: the suite id comes from the request, and only Block is supported. + merged.SuiteId = baseline.SuiteId; + merged.UnmockedToolPolicy = UnmockedToolPolicies.Block; + + Sanitise(merged, targets, existingCases, suite, warnings); + + var validationError = CaseValidation.Validate(merged); + var changes = Diff(baseline, merged); + + return new AgentTestAuthorResponse + { + Reply = attempt.Reply, + Draft = merged, + DraftChanged = changes.Count > 0, + Changes = changes, + ValidationErrors = validationError == null ? [] : [validationError], + Warnings = warnings + }; + } + + /// + /// Copies one declared field off the model's draft. An explicit switch rather than reflection: + /// which fields a model may write is a decision worth being able to read in one place, not one + /// that should emerge from property metadata and change whenever the DTO gains a field. + /// + private static void Apply(AgentTestCaseUpsertRequest target, AgentTestCaseUpsertRequest source, string field) + { + switch (field) + { + case AuthorFields.Name: target.Name = source.Name ?? string.Empty; break; + case AuthorFields.Enabled: target.Enabled = source.Enabled; break; + case AuthorFields.CaseType: target.CaseType = source.CaseType; break; + case AuthorFields.EntryAgentId: target.EntryAgentId = source.EntryAgentId; break; + case AuthorFields.Turns: target.Turns = source.Turns ?? []; break; + case AuthorFields.Assertions: target.Assertions = source.Assertions ?? []; break; + case AuthorFields.InitialStates: target.InitialStates = source.InitialStates ?? []; break; + case AuthorFields.History: target.History = source.History ?? []; break; + case AuthorFields.Mocks: target.Mocks = source.Mocks ?? []; break; + case AuthorFields.Priority: target.Priority = source.Priority; break; + case AuthorFields.Severity: target.Severity = source.Severity; break; + case AuthorFields.Batch: target.Batch = source.Batch; break; + case AuthorFields.CrossCutting: target.CrossCutting = source.CrossCutting; break; + case AuthorFields.InvolvedAgents: target.InvolvedAgents = source.InvolvedAgents ?? []; break; + case AuthorFields.BusinessDomain: target.BusinessDomain = source.BusinessDomain; break; + case AuthorFields.ExpectedOutcome: target.ExpectedOutcome = source.ExpectedOutcome; break; + } + } + + /// + /// Corrects what is provably wrong and flags what is merely suspect. + /// + /// The split matters. A function name is authoritative -- it comes from the agent definition, so a + /// mock naming something else can never match and is dropped. A state key is not: nothing in this + /// system enumerates the keys an agent writes (a state value records the message it was written + /// on and a coarse source, never a function name), so the only key list available is the one + /// other cases in this suite happen to use. Dropping a key for being absent from an admittedly + /// incomplete list would delete correct work, so an unknown key is a warning and stays. + /// + private static void Sanitise( + AgentTestCaseUpsertRequest draft, + List targets, + List existingCases, + AgentTestSuite suite, + List warnings) + { + var callable = new HashSet(targets.Select(t => t.Name), StringComparer.OrdinalIgnoreCase); + + var keptMocks = new List(); + foreach (var mock in draft.Mocks ?? []) + { + if (string.IsNullOrWhiteSpace(mock.FunctionName) || !callable.Contains(mock.FunctionName)) + { + warnings.Add($"dropped a mock for '{mock.FunctionName}': this agent has no such function, " + + "so the mock could never match at run time"); + continue; + } + + keptMocks.Add(mock); + } + draft.Mocks = keptMocks; + + var turns = new List(); + foreach (var (turn, index) in (draft.Turns ?? []).Select((t, i) => (t, i))) + { + // Re-indexed here as well as in the editor: the runner reads turns in order, and a model + // that renumbered them while inserting one would silently reorder the case. + turn.Index = index; + turn.Assertions = FilterAssertions(turn.Assertions, callable, $"turn {index + 1}", warnings); + turns.Add(turn); + } + draft.Turns = turns; + + draft.Assertions = FilterAssertions(draft.Assertions, callable, "the case", warnings); + + var knownStateKeys = new HashSet(StringComparer.OrdinalIgnoreCase); + foreach (var key in StateKeysOf(existingCases)) + { + knownStateKeys.Add(key); + } + + var allAssertions = (draft.Assertions ?? []) + .Concat((draft.Turns ?? []).SelectMany(t => t.Assertions ?? [])) + .ToList(); + + var assertedStateKeys = allAssertions + .Where(a => string.Equals(a.Type, AssertionTypes.StateEquals, StringComparison.Ordinal)) + .Select(a => a.Target) + .Where(k => !string.IsNullOrWhiteSpace(k)) + .Select(k => k!) + .Distinct(StringComparer.OrdinalIgnoreCase); + + foreach (var key in assertedStateKeys) + { + if (!knownStateKeys.Contains(key)) + { + warnings.Add($"state key '{key}' is not used by any other case in this suite; " + + "check it is the key the agent really writes"); + } + } + + var usesJudge = allAssertions.Any(a => string.Equals(a.Type, AssertionTypes.LlmJudge, StringComparison.Ordinal)); + if (usesJudge + && (string.IsNullOrWhiteSpace(suite.JudgeProvider) || string.IsNullOrWhiteSpace(suite.JudgeModel))) + { + warnings.Add("this draft uses llmJudge but the suite has no judge model configured, so that " + + "assertion will fail rather than pass when the case runs"); + } + } + + /// + /// Every conversation state key any case in the suite touches -- injected, written by a mock, or + /// asserted on. The nearest thing to a state key catalogue this system has; see + /// for why it is treated as incomplete. + /// + private static IEnumerable StateKeysOf(List cases) + => cases + .SelectMany(c => c.InitialStates.Select(s => s.Key) + .Concat(c.Mocks.SelectMany(m => m.StateWrites ?? []).Select(s => s.Key)) + .Concat(c.Assertions.Concat(c.Turns.SelectMany(t => t.Assertions)) + .Where(a => string.Equals(a.Type, AssertionTypes.StateEquals, StringComparison.Ordinal)) + .Select(a => a.Target ?? string.Empty))) + .Where(k => !string.IsNullOrWhiteSpace(k)) + .Distinct(StringComparer.OrdinalIgnoreCase) + .OrderBy(k => k, StringComparer.OrdinalIgnoreCase); + + private static List FilterAssertions( + List? assertions, + HashSet callable, + string where, + List warnings) + { + var kept = new List(); + + foreach (var assertion in assertions ?? []) + { + var isToolAssertion = + string.Equals(assertion.Type, AssertionTypes.ToolCalled, StringComparison.Ordinal) + || string.Equals(assertion.Type, AssertionTypes.ToolNotCalled, StringComparison.Ordinal); + + if (isToolAssertion && !string.IsNullOrWhiteSpace(assertion.Target) && !callable.Contains(assertion.Target!)) + { + warnings.Add($"dropped a {assertion.Type} assertion on {where}: this agent has no function " + + $"called '{assertion.Target}'"); + continue; + } + + kept.Add(assertion); + } + + return kept; + } + + /// + /// What actually changed, by comparing the two drafts field by field. Only whitelisted fields are + /// compared, because the merge cannot have moved anything else. + /// + private static List Diff(AgentTestCaseUpsertRequest before, AgentTestCaseUpsertRequest after) + { + var changes = new List(); + + foreach (var field in AuthorFields.All) + { + var oldValue = Read(before, field); + var newValue = Read(after, field); + + if (string.Equals(Json(oldValue), Json(newValue), StringComparison.Ordinal)) + { + continue; + } + + changes.Add(new AuthorChange { Field = field, Detail = Describe(oldValue, newValue) }); + } + + return changes; + } + + private static object? Read(AgentTestCaseUpsertRequest draft, string field) => field switch + { + AuthorFields.Name => draft.Name, + AuthorFields.Enabled => draft.Enabled, + AuthorFields.CaseType => draft.CaseType, + AuthorFields.EntryAgentId => draft.EntryAgentId, + AuthorFields.Turns => draft.Turns, + AuthorFields.Assertions => draft.Assertions, + AuthorFields.InitialStates => draft.InitialStates, + AuthorFields.History => draft.History, + AuthorFields.Mocks => draft.Mocks, + AuthorFields.Priority => draft.Priority, + AuthorFields.Severity => draft.Severity, + AuthorFields.Batch => draft.Batch, + AuthorFields.CrossCutting => draft.CrossCutting, + AuthorFields.InvolvedAgents => draft.InvolvedAgents, + AuthorFields.BusinessDomain => draft.BusinessDomain, + AuthorFields.ExpectedOutcome => draft.ExpectedOutcome, + _ => null + }; + + private static string Describe(object? before, object? after) + { + if (before is System.Collections.ICollection oldList && after is System.Collections.ICollection newList) + { + return oldList.Count == newList.Count + ? $"{newList.Count} item(s), edited" + : $"{oldList.Count} -> {newList.Count} item(s)"; + } + + return $"{Truncate(Text(before), 60)} -> {Truncate(Text(after), 60)}"; + } + + private static string Text(object? value) + => value is null || (value is string s && string.IsNullOrWhiteSpace(s)) + ? "(empty)" + : value.ToString() ?? "(empty)"; + + private static string Json(object? value) => JsonSerializer.Serialize(value, DraftJson); + + private static AgentTestCaseUpsertRequest Clone(AgentTestCaseUpsertRequest source) + => JsonSerializer.Deserialize(Json(source), DraftJson) + ?? new AgentTestCaseUpsertRequest(); + + private static string? ExtractJson(string raw) + { + var start = raw.IndexOf('{'); + var end = raw.LastIndexOf('}'); + return start >= 0 && end > start ? raw[start..(end + 1)] : null; + } + + /// + /// Fixes the one JSON-shape mistake models reliably make against this schema: writing a + /// "JSON as text" field -- an assertion or mock's argsMatchJson, a mock's resultContent, or a + /// state's value -- as a nested object or array instead of a string holding escaped JSON. Both + /// forms are syntactically valid JSON (which is why cannot catch this), + /// they only disagree on the CLR type the strongly-typed field expects, and re-serialising the + /// nested value back to text is the one lossless, unambiguous reading of what the model meant -- + /// unlike LlmAgentTestJudge.ParseVerdict's out-of-range score, there is no second plausible + /// interpretation here to guard against by rejecting instead of coercing. + /// + /// Silent about anything it does not recognise: a genuine JSON syntax error is left for the real + /// deserializer in to report, with the exact path and reason a retry needs. + /// + private static string CoerceStructuredJsonStringFields(string json) + { + JsonNode? root; + try + { + root = JsonNode.Parse(json); + } + catch (JsonException) + { + return json; + } + + if (root is not JsonObject envelope || FindObject(envelope, "draft") is not { } draft) + { + return json; + } + + foreach (var assertion in FindArrayOfObjects(draft, "assertions")) + { + StringifyIfStructured(assertion, "argsMatchJson"); + } + + foreach (var turn in FindArrayOfObjects(draft, "turns")) + { + foreach (var assertion in FindArrayOfObjects(turn, "assertions")) + { + StringifyIfStructured(assertion, "argsMatchJson"); + } + } + + foreach (var mock in FindArrayOfObjects(draft, "mocks")) + { + StringifyIfStructured(mock, "argsMatchJson"); + StringifyIfStructured(mock, "resultContent"); + + foreach (var write in FindArrayOfObjects(mock, "stateWrites")) + { + StringifyIfStructured(write, "value"); + } + } + + foreach (var state in FindArrayOfObjects(draft, "initialStates")) + { + StringifyIfStructured(state, "value"); + } + + return envelope.ToJsonString(); + } + + private static JsonObject? FindObject(JsonObject obj, string name) + => FindProperty(obj, name).Value as JsonObject; + + private static IEnumerable FindArrayOfObjects(JsonObject? obj, string name) + { + if (obj != null && FindProperty(obj, name).Value is JsonArray array) + { + foreach (var item in array) + { + if (item is JsonObject o) + { + yield return o; + } + } + } + } + + /// + /// Case-insensitive lookup: this runs before 's + /// PropertyNameCaseInsensitive gets a say, and a model that capitalises a field unexpectedly + /// should not skip normalisation just because the casing assumed here did not match verbatim. + /// + private static KeyValuePair FindProperty(JsonObject obj, string name) + => obj.FirstOrDefault(kv => string.Equals(kv.Key, name, StringComparison.OrdinalIgnoreCase)); + + private static void StringifyIfStructured(JsonObject obj, string propertyName) + { + var found = FindProperty(obj, propertyName); + if (found.Key != null && found.Value is JsonObject or JsonArray) + { + obj[found.Key] = JsonValue.Create(found.Value!.ToJsonString()); + } + } + + private static string Truncate(string text, int max) + => text.Length <= max ? text : text[..max].TrimEnd() + "..."; + + /// + /// The rules. The assertion vocabulary is generated from + /// rather than written out here, so a new assertion + /// type cannot exist in validation and be invisible to the author. + /// + private static string BuildInstruction() + { + var builder = new StringBuilder(); + + builder.AppendLine( + """ + You help a QA engineer write and edit ONE regression test case for a customer-service AI + agent, by conversation. You do not run tests and you do not save anything: you propose a + draft, and the human saves it. + + A case is: some turns of user messages, mocked tool returns so that nothing real happens, + and assertions that decide pass or fail. + + HOW TO ANSWER + - Reply in the language the user is writing in. + - `reply` is what you say to them: what you changed and why, or a question when their + request is genuinely ambiguous. Asking is better than guessing at a business rule. + - `changedFields` lists ONLY the top-level fields you are changing this turn. Fields you + do not list are kept exactly as they are, so listing a field you did not mean to touch + is how someone's work gets lost. + - When you are only answering a question or asking one, return an empty `changedFields`. + - Return the FULL new value of every field you list, never a fragment. To add one turn, + return all the turns including the new one. + + WHAT MAKES A GOOD ASSERTION + - Prefer assertions about what the agent DID: toolCalled, stateEquals, routedToAgent, + agentChain. Those survive a reworded reply. + - Avoid outputContains unless the user names the exact text that matters (an id format, a + required disclosure). Never write one from wording you imagined the agent would use -- + it fails the first time the model phrases things differently. + - For "is the reply any good" requirements use llmJudge, with the criterion in `expected`. + - Only use function names from CALLABLE FUNCTIONS. Never invent one. + - Only use state keys from KNOWN STATE KEYS, or ones the user gave you. If you need a key + you do not have, ask for it. + - Every tool the case will trigger needs a mock, or the run blocks the call and the case + fails. When a mocked function passes data to later turns through conversation state, + put that in the mock's stateWrites. + + FIELDS THAT HOLD JSON AS TEXT + argsMatchJson (on an assertion or a mock), a mock's resultContent, and any state "value" + are JSON STRINGS, not nested objects: their value must be the escaped JSON text, exactly + like this -- + "argsMatchJson": "{\"work_order_id\":\"12345\"}" + NOT this -- + "argsMatchJson": {"work_order_id": "12345"} + The second form is invalid for that field even though it is valid JSON overall. + + ASSERTION TYPES + """); + + foreach (var type in AssertionValidation.Authorable) + { + var required = AssertionValidation.RequiredFieldName(type); + var purpose = type switch + { + AssertionTypes.OutputContains => "the reply contains this text", + AssertionTypes.OutputNotContains => "the reply does not contain this text", + AssertionTypes.OutputRegex => "the reply matches this regular expression", + AssertionTypes.ToolCalled => "this function was called; the optional argsMatchJson is an argument subset", + AssertionTypes.ToolNotCalled => "this function was not called", + AssertionTypes.StateEquals => "conversation state at this key equals expected", + AssertionTypes.RoutedToAgent => "the agent named in expected handled the conversation", + AssertionTypes.AgentChain => "expected is a comma-separated agent list, target is contains|ordered|exact", + AssertionTypes.LlmJudge => "a model scores the reply 1-5 against the criterion in expected; minScore is the bar, 4 by default", + _ => "see the case editor" + }; + + builder.Append("- ").Append(type); + if (required != null) + { + builder.Append(" (requires `").Append(required).Append("`)"); + } + builder.Append(": ").AppendLine(purpose); + } + + builder.AppendLine( + """ + + CASE TYPE RULES + - caseType "Routing" means "did the router pick the right agent": exactly one turn, at + least one routedToAgent or agentChain assertion, and no llmJudge. + - caseType "Agent" is everything else, including multi-agent journeys. + - priority P0/P1/P2 decides which batch runs first; severity S0/S1/S2 says what a failure + means. Leave them at P1/S1 unless the user says otherwise. + + OUTPUT + Reply with JSON only. No prose outside it and no code fence: + {"reply":"...","changedFields":["turns"],"draft":{ the full case draft }} + + The draft uses exactly the field names shown in CURRENT DRAFT. + """); + + return builder.ToString(); + } + + private static string BuildContext( + Agent agent, + List targets, + List existingCases, + string? caseId, + AgentTestCaseResult? grounding, + List agentNames) + { + var builder = new StringBuilder(); + + builder.AppendLine("AGENT UNDER TEST").AppendLine($"name: {agent.Name}"); + + if (!string.IsNullOrWhiteSpace(agent.Description)) + { + builder.AppendLine($"description: {agent.Description}"); + } + + if (!string.IsNullOrWhiteSpace(agent.Instruction)) + { + // The most useful thing here -- it is where the business rules and the required slots + // live -- and also the longest, so it is capped rather than left to crowd out the + // function catalogue and the existing cases. + builder.AppendLine("instruction:").AppendLine(Truncate(agent.Instruction, MaxInstructionChars)); + } + + builder.AppendLine().AppendLine("CALLABLE FUNCTIONS (mock and assert against these names only)"); + if (targets.Count == 0) + { + builder.AppendLine("(none -- this agent calls no tools, so the case needs no mocks)"); + } + + foreach (var target in targets) + { + builder.Append("- ").Append(target.Name); + if (!string.IsNullOrWhiteSpace(target.Parameters)) + { + builder.Append(" [").Append(target.Parameters).Append(']'); + } + if (!string.IsNullOrWhiteSpace(target.Description)) + { + builder.Append(": ").Append(Truncate(target.Description!, 200)); + } + builder.AppendLine(); + } + + if (agentNames.Count > 0) + { + builder.AppendLine().AppendLine("AGENTS THAT EXIST (for routedToAgent and agentChain)") + .AppendLine(string.Join(", ", agentNames)); + } + + var stateKeys = StateKeysOf(existingCases).ToList(); + builder.AppendLine().AppendLine("KNOWN STATE KEYS (from other cases in this suite -- not a complete list)") + .AppendLine(stateKeys.Count == 0 ? "(none yet)" : string.Join(", ", stateKeys)); + + var others = existingCases + .Where(c => !string.Equals(c.Id, caseId, StringComparison.Ordinal)) + .Take(MaxExistingCases) + .ToList(); + + if (others.Count > 0) + { + // Names and shapes only, never the bodies: this exists so the model does not write a + // duplicate, and a full dump of every case would cost more than the whole rest of the + // prompt. + builder.AppendLine().AppendLine("CASES ALREADY IN THIS SUITE (do not duplicate them)"); + foreach (var other in others) + { + var assertionTypes = string.Join("/", other.Assertions + .Concat(other.Turns.SelectMany(t => t.Assertions)) + .Select(a => a.Type) + .Distinct(StringComparer.Ordinal)); + + builder.Append("- ").Append(other.Name) + .Append(" [").Append(other.CaseType).Append(", ").Append(other.Turns.Count).Append(" turn(s)"); + + if (!string.IsNullOrWhiteSpace(assertionTypes)) + { + builder.Append(", ").Append(assertionTypes); + } + + builder.AppendLine("]"); + } + } + + if (grounding != null) + { + builder.AppendLine().AppendLine( + $"WHAT HAPPENED LAST TIME THIS CASE RAN (status {grounding.Status}) -- real replies and real " + + "tool arguments. Base any outputContains or argsMatchJson on THIS, not on invention."); + + foreach (var turn in grounding.Turns.Take(MaxGroundedTurns)) + { + builder.Append("- turn ").Append(turn.Index).Append(" user: ").AppendLine(Truncate(turn.UserMessage, 200)); + + if (!string.IsNullOrWhiteSpace(turn.Output)) + { + builder.Append(" agent said: ").AppendLine(Truncate(turn.Output!, MaxGroundedOutputChars)); + } + + var failed = string.Join("; ", turn.Assertions + .Where(a => !a.Passed) + .Select(a => $"{a.Type} ({a.Message})")); + + if (!string.IsNullOrWhiteSpace(failed)) + { + builder.Append(" failed: ").AppendLine(Truncate(failed, 300)); + } + } + + foreach (var call in grounding.ObservedToolCalls.Take(MaxGroundedToolCalls)) + { + builder.Append("- turn ").Append(call.TurnIndex).Append(" called ").Append(call.FunctionName) + .Append(" [").Append(call.Outcome).Append(']'); + + if (!string.IsNullOrWhiteSpace(call.ArgsJson)) + { + builder.Append(" args: ").Append(Truncate(call.ArgsJson!, MaxGroundedArgsChars)); + } + + builder.AppendLine(); + } + } + + return builder.ToString(); + } + + private static string BuildTask(AgentTestCaseUpsertRequest draft, string instruction) + { + var builder = new StringBuilder(); + + builder.AppendLine("CURRENT DRAFT").AppendLine(JsonSerializer.Serialize(draft, DraftJson)).AppendLine(); + builder.AppendLine("WHAT TO DO").AppendLine(instruction).AppendLine(); + + // The format is restated here, not only in the instruction: the replayed assistant turns of + // this chat are plain prose (only the `reply` field is kept client-side), and without a + // reminder next to the actual request a model will follow that example and answer in prose. + builder.AppendLine( + """ + Answer with the JSON envelope only: + {"reply":"...","changedFields":[...],"draft":{...}} + """); + + return builder.ToString(); + } +} + +/// One parsed model answer, before it is merged or trusted. +/// What to show the user. +/// Recognised field names from changedFields. +/// changedFields entries naming no writable field. +/// The model's draft; only declared fields are ever read from it. +/// The raw reply, replayed to the model when asking it to repair a rejection. +public record AuthorAttempt( + string Reply, + List DeclaredFields, + List RejectedFields, + AgentTestCaseUpsertRequest? Draft, + string Raw); + +/// Wire shape of the model's answer. +internal class AuthorEnvelope +{ + public string? Reply { get; set; } + public List? ChangedFields { get; set; } + public AgentTestCaseUpsertRequest? Draft { get; set; } +} diff --git a/src/Plugins/BotSharp.Plugin.AgentTesting/Services/MockTargetCatalogue.cs b/src/Plugins/BotSharp.Plugin.AgentTesting/Services/MockTargetCatalogue.cs new file mode 100644 index 000000000..4a180e9d5 --- /dev/null +++ b/src/Plugins/BotSharp.Plugin.AgentTesting/Services/MockTargetCatalogue.cs @@ -0,0 +1,112 @@ +using BotSharp.Abstraction.Functions.Models; +using System.Text; +using System.Text.Json; + +namespace BotSharp.Plugin.AgentTesting.Services; + +/// +/// The functions an agent can actually call, derived live from the agent definition. +/// +/// Two shapes of the same list, deliberately kept together so they cannot drift: +/// is what GET /agent-test/mock-targets returns to the case editor, and +/// is the richer form puts in front of a model. +/// +/// Derived live rather than read from IFunctionCallback-full-detail-report.md on purpose: that +/// document is a point-in-time snapshot and drifts, and a mock authored against a function this +/// agent cannot call is a case that can never pass. +/// +public static class MockTargetCatalogue +{ + /// Function names only, sorted and de-duplicated. The wire shape the UI consumes. + public static List Names(Agent agent) + => Describe(agent).Select(t => t.Name).ToList(); + + /// + /// Every callable function with whatever description and parameter shape the agent definition + /// carries. + /// + /// MCP tools come back name-only: has nothing but a Name, so a model + /// authoring a mock for one is working from the name alone. That is a real gap, not an oversight + /// here -- it is why an MCP mock's argsMatchJson is more likely to need a human fix than a + /// plugin function's. + /// + public static List Describe(Agent agent) + { + var targets = new List(); + + foreach (var fn in (agent.Functions ?? []).Concat(agent.SecondaryFunctions ?? [])) + { + if (string.IsNullOrWhiteSpace(fn?.Name)) continue; + targets.Add(new MockTargetInfo(fn!.Name, fn.Description, ParameterSummary(fn.Parameters))); + } + + foreach (var fn in (agent.McpTools ?? []).SelectMany(t => t.Functions ?? [])) + { + if (string.IsNullOrWhiteSpace(fn?.Name)) continue; + targets.Add(new MockTargetInfo(fn!.Name, null, null)); + } + + // First entry wins on a duplicate name: a function declared both primary and secondary is + // one function, and the primary declaration is the one with the fuller definition. + return targets + .GroupBy(t => t.Name, StringComparer.OrdinalIgnoreCase) + .Select(g => g.First()) + .OrderBy(t => t.Name, StringComparer.OrdinalIgnoreCase) + .ToList(); + } + + /// + /// A one-line rendering of a function's parameters, e.g. work_order_id* (string), note, + /// with a star on the required ones. + /// + /// Flattened rather than passed as raw JSON schema: the whole catalogue goes into one prompt, and + /// full schemas for thirty functions crowd out the agent instruction and the existing cases -- + /// which are what actually make an authored case realistic. Names and types are enough for the + /// only thing the model writes with them, an argsMatchJson subset. + /// + private static string? ParameterSummary(FunctionParametersDef? parameters) + { + var properties = parameters?.Properties; + if (properties == null) return null; + + JsonElement root; + try + { + root = properties.RootElement; + } + catch (ObjectDisposedException) + { + // The agent definition is shared, and a JsonDocument someone else already disposed must + // not take down the authoring request with it. + return null; + } + + if (root.ValueKind != JsonValueKind.Object) return null; + + var required = parameters!.Required ?? []; + var parts = new List(); + + foreach (var property in root.EnumerateObject()) + { + var builder = new StringBuilder(property.Name); + if (required.Contains(property.Name, StringComparer.OrdinalIgnoreCase)) builder.Append('*'); + + if (property.Value.ValueKind == JsonValueKind.Object + && property.Value.TryGetProperty("type", out var type) + && type.ValueKind == JsonValueKind.String) + { + builder.Append(" (").Append(type.GetString()).Append(')'); + } + + parts.Add(builder.ToString()); + } + + return parts.Count == 0 ? null : string.Join(", ", parts); + } +} + +/// One callable function, as much as the agent definition knows about it. +/// The function name a mock or a toolCalled assertion has to match exactly. +/// Null for MCP tools -- see . +/// One-line parameter summary, or null when the function takes none. +public record MockTargetInfo(string Name, string? Description, string? Parameters); diff --git a/tests/BotSharp.Core.UnitTests/AgentTesting/AgentTestControllerTests.cs b/tests/BotSharp.Core.UnitTests/AgentTesting/AgentTestControllerTests.cs index 3a6bf8640..ffb6ef79d 100644 --- a/tests/BotSharp.Core.UnitTests/AgentTesting/AgentTestControllerTests.cs +++ b/tests/BotSharp.Core.UnitTests/AgentTesting/AgentTestControllerTests.cs @@ -147,7 +147,8 @@ private static AgentTestController BuildController( InMemoryRepo repo, out RecordingQueue queue, ILlmProviderService? llmProviders = null, - IAgentService? agents = null) + IAgentService? agents = null, + ICaseAuthor? author = null) { var recorder = new AgentTestRecorder( Mock.Of(), @@ -156,7 +157,8 @@ private static AgentTestController BuildController( queue = new RecordingQueue(); var controller = new AgentTestController( - repo, queue, agents ?? Mock.Of(), recorder, llmProviders ?? ProviderServiceKnowing()); + repo, queue, agents ?? Mock.Of(), recorder, author ?? Mock.Of(), + llmProviders ?? ProviderServiceKnowing()); // TriggerRun reads User.FindFirstValue(ClaimTypes.NameIdentifier) -- a directly-constructed // controller (no MVC pipeline/TestServer) has no HttpContext at all by default, and diff --git a/tests/BotSharp.Core.UnitTests/AgentTesting/CaseAuthoringTests.cs b/tests/BotSharp.Core.UnitTests/AgentTesting/CaseAuthoringTests.cs new file mode 100644 index 000000000..342283938 --- /dev/null +++ b/tests/BotSharp.Core.UnitTests/AgentTesting/CaseAuthoringTests.cs @@ -0,0 +1,522 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using BotSharp.Plugin.AgentTesting.Models; +using BotSharp.Plugin.AgentTesting.Services; +using Xunit; + +namespace BotSharp.Core.UnitTests.AgentTesting; + +/// +/// Authoring a case by conversation hands a model write access to a document a human is editing. The +/// four things stopping that from going wrong are all in and +/// , and this is where they are pinned: +/// +/// 1. a field the model does not declare cannot be deleted by being left out of its answer; +/// 2. a mock or tool assertion naming a function the agent cannot call is removed, not stored; +/// 3. a draft that would fail on save comes back labelled, not presented as progress; +/// 4. the diff shown to the user is computed from the two drafts, never read off the model's own +/// account of what it did. +/// +/// No vendor is involved: both methods are pure, which is why they are public. +/// +public class CaseAuthoringTests +{ + private static AgentTestSuite Suite(string? judgeProvider = null, string? judgeModel = null) => new() + { + Id = "suite-1", + AgentId = "agent-1", + Name = "suite", + Enabled = true, + JudgeProvider = judgeProvider, + JudgeModel = judgeModel + }; + + private static List Targets(params string[] names) + => names.Select(n => new MockTargetInfo(n, null, null)).ToList(); + + /// A saved case with one turn and one mock -- the thing an authoring turn can damage. + private static AgentTestCaseUpsertRequest Baseline() => new() + { + SuiteId = "suite-1", + Name = "where is my technician", + Turns = [new TestTurn { Index = 0, UserMessage = "where is my tech" }], + Mocks = [new TestToolMock { FunctionName = "get_eta", ResultContent = """{"eta":"2pm"}""" }] + }; + + private static AuthorAttempt Attempt( + string[] declared, + AgentTestCaseUpsertRequest? draft, + string reply = "Added a turn.", + string[]? rejected = null) + => new(reply, declared.ToList(), (rejected ?? []).ToList(), draft, "{}"); + + private static AgentTestAuthorResponse Run( + AuthorAttempt attempt, + AgentTestCaseUpsertRequest? baseline = null, + List? targets = null, + List? existing = null, + AgentTestSuite? suite = null) + => LlmCaseAuthor.Assemble( + baseline ?? Baseline(), + attempt, + targets ?? Targets("get_eta", "reschedule"), + existing ?? [], + suite ?? Suite()); + + // ---- 1. A field the model does not declare survives ------------------------------------- + + [Fact] + public void An_undeclared_field_is_kept_even_when_the_model_returns_it_empty() + { + // The whole reason for the declared-fields whitelist. A model that returns the document with + // `mocks` missing is the normal failure of asking for a full document back, and taking its + // answer wholesale would delete a mock nobody asked it to touch -- silently, because an + // absent mock only shows up as the case failing on its next run. + var modelDraft = new AgentTestCaseUpsertRequest + { + Name = "where is my technician", + Turns = + [ + new TestTurn { Index = 0, UserMessage = "where is my tech" }, + new TestTurn { Index = 1, UserMessage = "and the address?" } + ], + Mocks = [] + }; + + var response = Run(Attempt(["turns"], modelDraft)); + + Assert.Single(response.Draft.Mocks); + Assert.Equal("get_eta", response.Draft.Mocks[0].FunctionName); + Assert.Equal(2, response.Draft.Turns.Count); + } + + [Fact] + public void A_declared_field_is_applied() + { + var modelDraft = new AgentTestCaseUpsertRequest + { + Turns = + [ + new TestTurn { Index = 0, UserMessage = "where is my tech" }, + new TestTurn { Index = 1, UserMessage = "and the address?" } + ] + }; + + var response = Run(Attempt(["turns"], modelDraft)); + + Assert.True(response.DraftChanged); + Assert.Contains(response.Changes, c => c.Field == "turns"); + Assert.Equal("1 -> 2 item(s)", response.Changes.Single(c => c.Field == "turns").Detail); + } + + [Fact] + public void A_field_the_model_may_not_change_is_ignored_and_reported() + { + // AuthorFields.Normalize returns null for anything unwritable, and Parse routes those into + // RejectedFields. Reported rather than dropped in silence: "I renamed the suite for you" + // needs to be visibly untrue. + var response = Run(Attempt([], null, reply: "Renamed the suite.", rejected: ["suiteId"])); + + Assert.False(response.DraftChanged); + Assert.Contains(response.Warnings, w => w.Contains("suiteId")); + } + + [Fact] + public void The_unmocked_tool_policy_cannot_be_moved_off_block() + { + // Not a writable field, so a model proposing Passthrough gets Block anyway. Worth its own + // test because Passthrough is the one setting under which toolNotCalled passes vacuously + // against a tool that really executed. + var modelDraft = new AgentTestCaseUpsertRequest + { + Turns = [new TestTurn { Index = 0, UserMessage = "where is my tech" }], + UnmockedToolPolicy = "Passthrough" + }; + + var response = Run(Attempt(["turns"], modelDraft)); + + Assert.Equal(UnmockedToolPolicies.Block, response.Draft.UnmockedToolPolicy); + Assert.Empty(response.ValidationErrors); + } + + [Fact] + public void Declaring_a_field_without_returning_a_draft_changes_nothing() + { + var response = Run(Attempt(["turns"], null)); + + Assert.False(response.DraftChanged); + Assert.Single(response.Draft.Turns); + Assert.Contains(response.Warnings, w => w.Contains("returned no draft")); + } + + // ---- 2. Never a function the agent cannot call ------------------------------------------ + + [Fact] + public void A_mock_for_an_unknown_function_is_dropped() + { + var modelDraft = new AgentTestCaseUpsertRequest + { + Mocks = + [ + new TestToolMock { FunctionName = "get_eta", ResultContent = "{}" }, + new TestToolMock { FunctionName = "send_sms_to_resident", ResultContent = "{}" } + ] + }; + + var response = Run(Attempt(["mocks"], modelDraft)); + + Assert.Single(response.Draft.Mocks); + Assert.Equal("get_eta", response.Draft.Mocks[0].FunctionName); + Assert.Contains(response.Warnings, w => w.Contains("send_sms_to_resident")); + } + + [Fact] + public void A_tool_assertion_on_an_unknown_function_is_dropped() + { + // Kept, it would never match at run time, so the case would fail forever for a reason that + // reads as an agent regression. + var modelDraft = new AgentTestCaseUpsertRequest + { + Assertions = + [ + new TestAssertion { Type = AssertionTypes.ToolCalled, Target = "get_eta" }, + new TestAssertion { Type = AssertionTypes.ToolCalled, Target = "get_estimated_arrival" } + ] + }; + + var response = Run(Attempt(["assertions"], modelDraft)); + + Assert.Single(response.Draft.Assertions); + Assert.Equal("get_eta", response.Draft.Assertions[0].Target); + Assert.Contains(response.Warnings, w => w.Contains("get_estimated_arrival")); + } + + [Fact] + public void A_non_tool_assertion_is_never_dropped_for_its_target() + { + // stateEquals also carries a Target, and it is a state key, not a function name. + var modelDraft = new AgentTestCaseUpsertRequest + { + Assertions = [new TestAssertion { Type = AssertionTypes.StateEquals, Target = "wo_num", Expected = "B1" }] + }; + + var response = Run(Attempt(["assertions"], modelDraft)); + + Assert.Single(response.Draft.Assertions); + } + + [Fact] + public void An_unfamiliar_state_key_is_kept_but_flagged() + { + // The asymmetry with function names is deliberate: nothing in this system enumerates the keys + // an agent writes, so the "known" list is only what other cases happen to use. Dropping on + // absence from an admittedly incomplete list would delete correct work. + var modelDraft = new AgentTestCaseUpsertRequest + { + Assertions = [new TestAssertion { Type = AssertionTypes.StateEquals, Target = "invented_key", Expected = "x" }] + }; + + var response = Run(Attempt(["assertions"], modelDraft)); + + Assert.Single(response.Draft.Assertions); + Assert.Contains(response.Warnings, w => w.Contains("invented_key")); + } + + [Fact] + public void A_state_key_another_case_already_uses_is_not_flagged() + { + var existing = new List + { + new() + { + Id = "case-9", + SuiteId = "suite-1", + Name = "other", + InitialStates = [new TestState { Key = "wo_num", Value = "B1" }] + } + }; + + var modelDraft = new AgentTestCaseUpsertRequest + { + Assertions = [new TestAssertion { Type = AssertionTypes.StateEquals, Target = "wo_num", Expected = "B1" }] + }; + + var response = Run(Attempt(["assertions"], modelDraft), existing: existing); + + Assert.DoesNotContain(response.Warnings, w => w.Contains("wo_num")); + } + + [Fact] + public void An_llm_judge_assertion_on_a_suite_with_no_judge_model_is_flagged() + { + // It would not fail validation -- it would save cleanly and then fail every run, which is the + // sort of thing worth saying at authoring time. + var modelDraft = new AgentTestCaseUpsertRequest + { + Assertions = [new TestAssertion { Type = AssertionTypes.LlmJudge, Expected = "polite and specific" }] + }; + + var withoutJudge = Run(Attempt(["assertions"], modelDraft)); + Assert.Contains(withoutJudge.Warnings, w => w.Contains("judge model")); + + var withJudge = Run(Attempt(["assertions"], modelDraft), suite: Suite("openai", "gpt-4o")); + Assert.DoesNotContain(withJudge.Warnings, w => w.Contains("judge model")); + } + + // ---- 3. An invalid draft is labelled, not presented as progress ------------------------- + + [Fact] + public void A_routing_case_the_model_gave_two_turns_comes_back_with_the_validation_error() + { + var modelDraft = new AgentTestCaseUpsertRequest + { + CaseType = CaseTypes.Routing, + Turns = + [ + new TestTurn { Index = 0, UserMessage = "my fridge leaks" }, + new TestTurn { Index = 1, UserMessage = "when will someone come" } + ], + Assertions = [new TestAssertion { Type = AssertionTypes.RoutedToAgent, Expected = "Work Order Agent" }] + }; + + var response = Run(Attempt(["caseType", "turns", "assertions"], modelDraft)); + + Assert.NotEmpty(response.ValidationErrors); + Assert.Contains("exactly one turn", response.ValidationErrors[0]); + } + + [Fact] + public void A_routing_case_that_asserts_no_routing_outcome_is_rejected() + { + var modelDraft = new AgentTestCaseUpsertRequest + { + CaseType = CaseTypes.Routing, + Turns = [new TestTurn { Index = 0, UserMessage = "my fridge leaks" }], + Assertions = [new TestAssertion { Type = AssertionTypes.OutputContains, Expected = "sorry" }] + }; + + var response = Run(Attempt(["caseType", "turns", "assertions"], modelDraft)); + + Assert.NotEmpty(response.ValidationErrors); + } + + [Fact] + public void Turns_are_renumbered_from_zero() + { + // A model inserting a turn tends to renumber badly or not at all, and the runner reads turns + // in order -- so a stale Index would reorder the case rather than fail it. + var modelDraft = new AgentTestCaseUpsertRequest + { + Turns = + [ + new TestTurn { Index = 5, UserMessage = "first" }, + new TestTurn { Index = 5, UserMessage = "second" } + ] + }; + + var response = Run(Attempt(["turns"], modelDraft)); + + Assert.Equal([0, 1], response.Draft.Turns.Select(t => t.Index)); + } + + // ---- 4. The diff is computed, not claimed ----------------------------------------------- + + [Fact] + public void A_declared_field_that_did_not_actually_change_is_not_reported_as_changed() + { + // The model claims two fields; only one differs. What the user sees has to be the second + // number, or the change list stops being evidence of anything. + var modelDraft = new AgentTestCaseUpsertRequest + { + Name = "where is my technician", + Turns = + [ + new TestTurn { Index = 0, UserMessage = "where is my tech" }, + new TestTurn { Index = 1, UserMessage = "and the address?" } + ] + }; + + var response = Run(Attempt(["name", "turns"], modelDraft)); + + Assert.Contains(response.Changes, c => c.Field == "turns"); + Assert.DoesNotContain(response.Changes, c => c.Field == "name"); + } + + [Fact] + public void An_answer_that_declares_nothing_is_a_question_not_a_no_op_edit() + { + // The clarifying-question path. It has to leave the draft untouched and still deliver the + // reply, or the model cannot ask anything without also having to invent an edit. + var response = Run(Attempt([], null, reply: "Which work order should the case use?")); + + Assert.False(response.DraftChanged); + Assert.Empty(response.Changes); + Assert.Equal("Which work order should the case use?", response.Reply); + Assert.Single(response.Draft.Turns); + Assert.Single(response.Draft.Mocks); + } + + [Fact] + public void The_suite_id_always_comes_from_the_request() + { + var modelDraft = new AgentTestCaseUpsertRequest + { + SuiteId = "some-other-suite", + Turns = [new TestTurn { Index = 0, UserMessage = "hi" }] + }; + + var response = Run(Attempt(["turns"], modelDraft)); + + Assert.Equal("suite-1", response.Draft.SuiteId); + } + + // ---- Parse: reading the envelope -------------------------------------------------------- + + [Fact] + public void Parse_normalises_declared_field_names_and_separates_the_unwritable_ones() + { + var attempt = LlmCaseAuthor.Parse( + """{"reply":"ok","changedFields":["Turns","lastReviewedDate","turns"],"draft":{"name":"x"}}"""); + + Assert.Equal("ok", attempt.Reply); + Assert.Equal(["turns"], attempt.DeclaredFields); + Assert.Equal(["lastReviewedDate"], attempt.RejectedFields); + Assert.Equal("x", attempt.Draft?.Name); + } + + [Fact] + public void Parse_tolerates_a_code_fence_and_surrounding_prose() + { + // The commonest and most harmless way for a model to disobey "JSON only" -- the segmenter + // takes the same view. + var attempt = LlmCaseAuthor.Parse( + "Here you go:\n```json\n{\"reply\":\"ok\",\"changedFields\":[],\"draft\":null}\n```"); + + Assert.Equal("ok", attempt.Reply); + Assert.Empty(attempt.DeclaredFields); + } + + [Fact] + public void Parse_treats_a_missing_changed_fields_list_as_no_change() + { + var attempt = LlmCaseAuthor.Parse("""{"reply":"I need to know the work order number first."}"""); + + Assert.Empty(attempt.DeclaredFields); + Assert.Null(attempt.Draft); + } + + [Theory] + [InlineData("I could not do that.")] + [InlineData("")] + [InlineData("{ not json at all ")] + public void Parse_rejects_an_answer_it_cannot_read(string raw) + { + // No draft was produced, which is a different outcome from an invalid draft: the caller + // should retry, not review. + Assert.Throws(() => LlmCaseAuthor.Parse(raw)); + } + + // ---- Parse: coercing a JSON-as-text field written as a nested object ------------------- + // + // The exact shape reported in production: a model writes argsMatchJson as a real object because + // that is the natural way to express "match these arguments", even though the field is a string + // holding escaped JSON. Both forms are syntactically valid JSON, so nothing before deserialization + // can reject this as malformed -- it has to be normalised before the strongly-typed model ever + // sees it, or every case-level and turn-level argsMatchJson blows up the whole reply. + + [Fact] + public void An_object_valued_argsMatchJson_on_a_case_level_assertion_is_coerced_to_its_json_text() + { + var attempt = LlmCaseAuthor.Parse( + """ + {"reply":"ok","changedFields":["assertions"],"draft":{"assertions":[ + {"type":"toolCalled","target":"get_eta","argsMatchJson":{"work_order_id":"12345"}} + ]}} + """); + + var assertion = Assert.Single(attempt.Draft!.Assertions); + Assert.Equal("""{"work_order_id":"12345"}""", assertion.ArgsMatchJson); + } + + [Fact] + public void An_object_valued_argsMatchJson_on_a_turn_level_assertion_is_also_coerced() + { + var attempt = LlmCaseAuthor.Parse( + """ + {"reply":"ok","changedFields":["turns"],"draft":{"turns":[ + {"index":0,"userMessage":"where is my tech","assertions":[ + {"type":"toolCalled","target":"get_eta","argsMatchJson":{"wo_num":"B1"}} + ]} + ]}} + """); + + var assertion = Assert.Single(attempt.Draft!.Turns[0].Assertions); + Assert.Equal("""{"wo_num":"B1"}""", assertion.ArgsMatchJson); + } + + [Fact] + public void An_object_valued_mock_result_content_and_args_match_are_both_coerced() + { + var attempt = LlmCaseAuthor.Parse( + """ + {"reply":"ok","changedFields":["mocks"],"draft":{"mocks":[ + {"functionName":"get_eta","argsMatchJson":{"wo_num":"B1"},"resultContent":{"eta":"2pm"}} + ]}} + """); + + var mock = Assert.Single(attempt.Draft!.Mocks); + Assert.Equal("""{"wo_num":"B1"}""", mock.ArgsMatchJson); + Assert.Equal("""{"eta":"2pm"}""", mock.ResultContent); + } + + [Fact] + public void An_array_valued_mock_state_write_value_is_coerced() + { + var attempt = LlmCaseAuthor.Parse( + """ + {"reply":"ok","changedFields":["mocks"],"draft":{"mocks":[ + {"functionName":"get_eta","stateWrites":[{"key":"items","value":[1,2,3]}]} + ]}} + """); + + Assert.Equal("[1,2,3]", attempt.Draft!.Mocks[0].StateWrites![0].Value); + } + + [Fact] + public void An_object_valued_initial_state_value_is_coerced() + { + var attempt = LlmCaseAuthor.Parse( + """ + {"reply":"ok","changedFields":["initialStates"],"draft":{"initialStates":[ + {"key":"customer","value":{"name":"Jane"}} + ]}} + """); + + Assert.Equal("""{"name":"Jane"}""", attempt.Draft!.InitialStates[0].Value); + } + + [Fact] + public void A_string_valued_argsMatchJson_passes_through_unchanged() + { + // The correct, already-escaped form must not be touched -- only the wrong shape is coerced. + var attempt = LlmCaseAuthor.Parse( + """ + {"reply":"ok","changedFields":["assertions"],"draft":{"assertions":[ + {"type":"toolCalled","target":"get_eta","argsMatchJson":"{\"wo_num\":\"B1\"}"} + ]}} + """); + + Assert.Equal("""{"wo_num":"B1"}""", attempt.Draft!.Assertions[0].ArgsMatchJson); + } + + [Fact] + public void A_genuine_json_syntax_error_still_fails_instead_of_being_silently_swallowed() + { + // The coercion pass must not mask a real syntax error by quietly returning the input + // unchanged and hoping for the best -- it has to fall through to the same rejection an + // unrecoverable reply always gets. + Assert.Throws(() => LlmCaseAuthor.Parse( + """{"reply":"ok","changedFields":["assertions"],"draft":{"assertions":[{,}]}}""")); + } +}