From 2c86bb6d316f52841018b3caecd93e3d24a9eb28 Mon Sep 17 00:00:00 2001 From: arst Date: Fri, 4 Sep 2026 10:35:05 +0200 Subject: [PATCH] Fix repository review findings --- .../ReliableChannel.cs | 28 ++++++-- AgenticPatterns.Tests/CasePartitionTests.cs | 9 +++ .../NewContextPatternTests.cs | 9 +++ .../NewProductionControlTests.cs | 64 +++++++++++++++++++ .../ProductionControlTests.cs | 30 +++++++-- .../ProductionControlsPhaseTwoTests.cs | 15 +++++ DualLlm.AgentFramework/DataFlow.cs | 36 +++++++++-- .../TraceReplay.cs | 17 ++++- GuardRails.AgentFramework/GuardRails.cs | 58 ++++++++--------- GuardRails.AgentFramework/Program.cs | 4 +- .../EpisodicStore.cs | 2 +- .../patterns/MemoryConsolidation.md | 4 +- .../CasePartition.cs | 2 +- 13 files changed, 222 insertions(+), 56 deletions(-) diff --git a/AgentCommunicationFaultTolerance.AgentFramework/ReliableChannel.cs b/AgentCommunicationFaultTolerance.AgentFramework/ReliableChannel.cs index 760c05d..1ba64c5 100644 --- a/AgentCommunicationFaultTolerance.AgentFramework/ReliableChannel.cs +++ b/AgentCommunicationFaultTolerance.AgentFramework/ReliableChannel.cs @@ -24,17 +24,28 @@ public sealed class FlakyTransport(int seed, double lossRate, double duplicateRa public sealed class Inbox { readonly Dictionary handled = new(StringComparer.Ordinal); + readonly object gate = new(); - public IReadOnlyDictionary Handled => handled; + public IReadOnlyDictionary Handled + { + get + { + lock (gate) return new Dictionary(handled, StringComparer.Ordinal); + } + } /// Returns the effect's result and whether this was a replay rather than a first delivery. public (string Result, bool Duplicate) Handle(Message message, Func effect) { - if (handled.TryGetValue(message.Id, out var existing)) return (existing, true); + // ponytail: one global lock; use per-message locks if unrelated effects need parallel throughput. + lock (gate) + { + if (handled.TryGetValue(message.Id, out var existing)) return (existing, true); - var result = effect(message); - handled[message.Id] = result; - return (result, false); + var result = effect(message); + handled[message.Id] = result; + return (result, false); + } } } @@ -84,6 +95,9 @@ public async Task SendAsync(Message message, Func eff /// The step people skip. Retries and dead-letters make each message's fate correct; only a /// reconciliation pass makes the CONVERSATION correct - it is where you find out that agent B /// is missing the one message agent A believes it sent. - public static IReadOnlyList Reconcile(IEnumerable sent, Inbox inbox) => - [.. sent.Select(m => m.Id).Where(id => !inbox.Handled.ContainsKey(id))]; + public static IReadOnlyList Reconcile(IEnumerable sent, Inbox inbox) + { + var handled = inbox.Handled; + return [.. sent.Select(m => m.Id).Where(id => !handled.ContainsKey(id))]; + } } diff --git a/AgenticPatterns.Tests/CasePartitionTests.cs b/AgenticPatterns.Tests/CasePartitionTests.cs index 5a1a84d..b0dd9b8 100644 --- a/AgenticPatterns.Tests/CasePartitionTests.cs +++ b/AgenticPatterns.Tests/CasePartitionTests.cs @@ -38,6 +38,15 @@ public void EmptyReviewedByIsAlsoAwaitingReview() Assert.Single(awaitingReview); } + [Fact] + public void WhitespaceReviewedByIsAlsoAwaitingReview() + { + var (evaluated, awaitingReview) = CasePartition.Partition([Case("blank", reviewedBy: " \t")]); + + Assert.Empty(evaluated); + Assert.Single(awaitingReview); + } + [Fact] public void MixedCorpusSplitsCorrectly() { diff --git a/AgenticPatterns.Tests/NewContextPatternTests.cs b/AgenticPatterns.Tests/NewContextPatternTests.cs index c0c2d59..11a2608 100644 --- a/AgenticPatterns.Tests/NewContextPatternTests.cs +++ b/AgenticPatterns.Tests/NewContextPatternTests.cs @@ -196,6 +196,15 @@ public void RecencyDecaysWithAge() Assert.True(scored[0].Recency > scored[1].Recency); } + [Fact] + public void DayOldRecencyMatchesTheDocumentedDecay() + { + var scored = EpisodicRetrieval.Score( + [new("ep-1", "memory", Now.AddDays(-1), 0.5, "t")], "unrelated", Now); + + Assert.InRange(scored.Single().Recency, 0.88, 0.89); + } + [Fact] public void OnlyTopicsOverTheThresholdConsolidate() { diff --git a/AgenticPatterns.Tests/NewProductionControlTests.cs b/AgenticPatterns.Tests/NewProductionControlTests.cs index da0fbf4..da226b7 100644 --- a/AgenticPatterns.Tests/NewProductionControlTests.cs +++ b/AgenticPatterns.Tests/NewProductionControlTests.cs @@ -30,6 +30,47 @@ public void AWellFormedChainPasses() => new Step("file_expense", ["total"], "receipt", "text") ], Tools)); + [Fact] + public void ToolContractsRejectWrongArityAndOutputTypes() + { + var errors = DataFlowPlan.Validate( + [ + new Step("fetch_email", [], "email", "text"), + new Step("extract_total", [], "total", "decimal"), + new Step("file_expense", ["total"], "receipt", "text") + ], Tools); + + Assert.Contains(errors, error => error.Message.Contains("produces 'untrusted_text'")); + Assert.Contains(errors, error => error.Message.Contains("expects 1 argument")); + } + + [Fact] + public void ConsumerInputTypesMustMatchTheirProducer() + { + var errors = DataFlowPlan.Validate( + [ + new Step("fetch_email", [], "email", "untrusted_text"), + new Step("extract_total", ["email"], "total", "decimal"), + new Step("file_expense", ["email"], "receipt", "text") + ], Tools); + + Assert.Contains(errors, error => error.Message.Contains("expected 'decimal'")); + } + + [Fact] + public void APlanMustHaveExactlyOneSideEffectingSink() + { + var errors = DataFlowPlan.Validate( + [ + new Step("fetch_email", [], "email", "untrusted_text"), + new Step("extract_total", ["email"], "total", "decimal"), + new Step("file_expense", ["total"], "receipt-1", "text"), + new Step("file_expense", ["total"], "receipt-2", "text") + ], Tools); + + Assert.Contains(errors, error => error.Message.Contains("exactly one 'file_expense'")); + } + [Fact] public void ReassigningAVariableIsRejected() => Assert.NotEmpty(DataFlowPlan.Validate( @@ -186,6 +227,29 @@ public async Task ADuplicateDeliveryRunsTheEffectOnce() Assert.Equal(1, runs); } + [Fact] + public async Task ConcurrentDuplicateDeliveriesRunTheEffectOnce() + { + const int callers = 8; + using var start = new Barrier(callers); + var runs = 0; + var inbox = new Inbox(); + + await Task.WhenAll(Enumerable.Range(0, callers).Select(_ => Task.Run(() => + { + start.SignalAndWait(); + inbox.Handle(Message("M1"), _ => + { + Interlocked.Increment(ref runs); + Thread.Sleep(50); + return "done"; + }); + }))); + + Assert.Equal(1, runs); + Assert.Single(inbox.Handled); + } + [Fact] public async Task ARetriedMessageStillOnlyRunsTheEffectOnce() { diff --git a/AgenticPatterns.Tests/ProductionControlTests.cs b/AgenticPatterns.Tests/ProductionControlTests.cs index 94e2d6f..34e96ae 100644 --- a/AgenticPatterns.Tests/ProductionControlTests.cs +++ b/AgenticPatterns.Tests/ProductionControlTests.cs @@ -570,22 +570,42 @@ public void RedactionCopiesResponseMetadata() } [Fact] - public void TruncateTrimsOnlyTheLastTextContent() + public void TruncateKeepsThePrefixAndCountsTheMarkerWithinTheLimit() { var functionCall = new FunctionCallContent("c1", "lookup", null); var response = new ChatResponse([ new ChatMessage(ChatRole.Assistant, [new TextContent("first ten.")]), - new ChatMessage(ChatRole.Assistant, [functionCall, new TextContent("second block of text")]) + new ChatMessage(ChatRole.Assistant, + [functionCall, new TextContent("second block of text that keeps going for much longer")]) ]); - // 10 ("first ten.") + budget 5 left for the last TextContent out of a 15-character cap. - var truncated = GuardRails.Truncate(response, maxCharacters: 15); + var truncated = GuardRails.Truncate(response, maxCharacters: 50); Assert.Equal("first ten.", ((TextContent)truncated.Messages[0].Contents[0]).Text); Assert.Same(functionCall, truncated.Messages[1].Contents[0]); var lastText = ((TextContent)truncated.Messages[1].Contents[1]).Text; - Assert.StartsWith("secon", lastText); + Assert.StartsWith("second", lastText); Assert.Contains("[Response truncated for safety.]", lastText); + Assert.True(truncated.Messages.Sum(message => + message.Contents.OfType().Sum(text => text.Text.Length)) <= 50); + } + + [Fact] + public void TruncateCapsEarlierTextAndClearsLaterText() + { + var functionCall = new FunctionCallContent("c1", "lookup", null); + var response = new ChatResponse([ + new ChatMessage(ChatRole.Assistant, new string('x', 100)), + new ChatMessage(ChatRole.Assistant, [functionCall, new TextContent("later")]) + ]); + + var truncated = GuardRails.Truncate(response, maxCharacters: 50); + + Assert.Equal(50, truncated.Messages.Sum(message => + message.Contents.OfType().Sum(text => text.Text.Length))); + Assert.Contains("[Response truncated for safety.]", truncated.Messages[0].Text); + Assert.Equal("", truncated.Messages[1].Text); + Assert.Same(functionCall, truncated.Messages[1].Contents[0]); } [Fact] diff --git a/AgenticPatterns.Tests/ProductionControlsPhaseTwoTests.cs b/AgenticPatterns.Tests/ProductionControlsPhaseTwoTests.cs index b35d353..fcfa465 100644 --- a/AgenticPatterns.Tests/ProductionControlsPhaseTwoTests.cs +++ b/AgenticPatterns.Tests/ProductionControlsPhaseTwoTests.cs @@ -107,6 +107,21 @@ await Assert.ThrowsAsync(() => replay.GetResponseAsync([new ChatMessage(ChatRole.User, "changed")])); } + [Fact] + public void RequestHashIncludesGenerationOptions() + { + var messages = new[] { new ChatMessage(ChatRole.User, "same prompt") }; + var original = TraceStore.HashMessages(messages, + new ChatOptions { Instructions = "v1", MaxOutputTokens = 100 }, TracePrivacyMode.FullContent); + var changedInstructions = TraceStore.HashMessages(messages, + new ChatOptions { Instructions = "v2", MaxOutputTokens = 100 }, TracePrivacyMode.FullContent); + var changedLimit = TraceStore.HashMessages(messages, + new ChatOptions { Instructions = "v1", MaxOutputTokens = 500 }, TracePrivacyMode.FullContent); + + Assert.NotEqual(original, changedInstructions); + Assert.NotEqual(original, changedLimit); + } + [Fact] public async Task TraceFileRoundTrips() { diff --git a/DualLlm.AgentFramework/DataFlow.cs b/DualLlm.AgentFramework/DataFlow.cs index ca58c08..a88a7fa 100644 --- a/DualLlm.AgentFramework/DataFlow.cs +++ b/DualLlm.AgentFramework/DataFlow.cs @@ -15,6 +15,14 @@ public sealed record PlanError(string Step, string Message); public static class DataFlowPlan { + private static readonly IReadOnlyDictionary Contracts = + new Dictionary(StringComparer.Ordinal) + { + ["fetch_email"] = ([], "untrusted_text"), + ["extract_total"] = (["untrusted_text"], "decimal"), + ["file_expense"] = (["decimal"], "text") + }; + /// The plan is written by the privileged model, which has seen only the user's instruction - /// but "privileged" describes what it was shown, not that its output is trusted. Validate the /// whole plan before a single step runs. @@ -22,22 +30,42 @@ public static IReadOnlyList Validate(IReadOnlyList steps, IReadOnlySet allowedTools) { var errors = new List(); - var produced = new HashSet(StringComparer.Ordinal); + var produced = new Dictionary(StringComparer.Ordinal); foreach (var step in steps) { if (!allowedTools.Contains(step.Tool)) errors.Add(new PlanError(step.Tool, $"tool '{step.Tool}' is not allowed")); - foreach (var arg in step.Args) - if (!produced.Contains(arg)) + var hasContract = Contracts.TryGetValue(step.Tool, out var contract); + if (!hasContract) + errors.Add(new PlanError(step.Tool, $"tool '{step.Tool}' has no declared data-flow contract")); + else if (step.Args.Length != contract.Inputs.Length) + errors.Add(new PlanError(step.Tool, + $"tool '{step.Tool}' expects {contract.Inputs.Length} argument(s), got {step.Args.Length}")); + + for (var i = 0; i < step.Args.Length; i++) + { + var arg = step.Args[i]; + if (!produced.TryGetValue(arg, out var actualType)) errors.Add(new PlanError(step.Tool, $"argument '{arg}' is not a variable produced by an earlier step")); + else if (hasContract && i < contract.Inputs.Length && actualType != contract.Inputs[i]) + errors.Add(new PlanError(step.Tool, + $"argument '{arg}' has type '{actualType}', expected '{contract.Inputs[i]}'")); + } - if (!produced.Add(step.Produces)) + if (hasContract && step.ProducesType != contract.Output) + errors.Add(new PlanError(step.Tool, + $"tool '{step.Tool}' produces '{contract.Output}', not '{step.ProducesType}'")); + + if (!produced.TryAdd(step.Produces, step.ProducesType)) errors.Add(new PlanError(step.Tool, $"variable '{step.Produces}' is assigned twice")); } + if (steps.Count(step => step.Tool == "file_expense") != 1) + errors.Add(new PlanError("file_expense", "plan must contain exactly one 'file_expense' step")); + return errors; } diff --git a/EvaluationAndMonitoring.AgentFramework/TraceReplay.cs b/EvaluationAndMonitoring.AgentFramework/TraceReplay.cs index d7f3b76..3c198db 100644 --- a/EvaluationAndMonitoring.AgentFramework/TraceReplay.cs +++ b/EvaluationAndMonitoring.AgentFramework/TraceReplay.cs @@ -212,7 +212,22 @@ public static string CanonicalOptions(ChatOptions? options) var tools = string.Join(";", options?.Tools?.Select(tool => tool is AIFunctionDeclaration function ? $"{function.Name}:{function.JsonSchema.GetRawText()}" : $"{tool.Name}:{tool.Description}") ?? []); - return $"model:{options?.ModelId}|temperature:{options?.Temperature}|format:{options?.ResponseFormat}|tools:{tools}"; + var toolMode = options?.ToolMode is RequiredChatToolMode required + ? $"Required:{required.RequiredFunctionName}" + : options?.ToolMode?.GetType().Name ?? ""; + var additionalProperties = options?.AdditionalProperties is { } props + ? JsonSerializer.Serialize(props.OrderBy(p => p.Key, StringComparer.Ordinal)) + : ""; + + return string.Join('|', + $"model:{options?.ModelId}", $"temperature:{options?.Temperature}", $"format:{options?.ResponseFormat}", + $"tools:{tools}", $"toolMode:{toolMode}", $"allowMultipleToolCalls:{options?.AllowMultipleToolCalls}", + $"instructions:{options?.Instructions}", $"maxOutputTokens:{options?.MaxOutputTokens}", + $"topP:{options?.TopP}", $"topK:{options?.TopK}", $"seed:{options?.Seed}", + $"stopSequences:{string.Join(",", options?.StopSequences ?? [])}", + $"frequencyPenalty:{options?.FrequencyPenalty}", $"presencePenalty:{options?.PresencePenalty}", + $"reasoningEffort:{options?.Reasoning?.Effort}", $"reasoningOutput:{options?.Reasoning?.Output}", + $"additionalProperties:{additionalProperties}"); } private static string Redact(string value) => Regex.Replace( diff --git a/GuardRails.AgentFramework/GuardRails.cs b/GuardRails.AgentFramework/GuardRails.cs index 9d0ee29..e51d27d 100644 --- a/GuardRails.AgentFramework/GuardRails.cs +++ b/GuardRails.AgentFramework/GuardRails.cs @@ -18,10 +18,8 @@ public static ChatResponse Redact(ChatResponse response) => WithMessages(response, RedactMessages(response.Messages)); /// - /// Caps the response's total text at . Only the last - /// in the response absorbs the cut — earlier text and every - /// non-text content item are left untouched. See for the - /// exact budget rule. + /// Caps the response's total text at , including the + /// truncation marker. Non-text content items are left untouched. /// public static ChatResponse Truncate(ChatResponse response, int maxCharacters) => WithMessages(response, TruncateMessages(response.Messages, maxCharacters)); @@ -41,46 +39,40 @@ internal static ChatMessage RedactMessage(ChatMessage message) => /// /// Truncates a list of messages to at most characters of /// total text. "Total text" is the sum of every length across - /// every message. When that sum exceeds the limit, only the last message's last - /// is shortened — down to whatever budget remains once every other - /// text item is counted — and a truncation marker is appended; everything before it, and - /// every non-text content item, is untouched. Returns the original list unchanged (same - /// reference) when already under budget, so callers can detect a no-op with + /// every message. When that sum exceeds the limit, the response prefix is kept, a truncation + /// marker is appended within the budget, and later text is cleared. Non-text content is + /// untouched. Returns the original list unchanged (same reference) when already under budget, + /// so callers can detect a no-op with /// . /// internal static IList TruncateMessages(IList messages, int maxCharacters) { + ArgumentOutOfRangeException.ThrowIfNegative(maxCharacters); + var totalLength = messages.Sum(m => m.Contents.OfType().Sum(t => t.Text.Length)); if (totalLength <= maxCharacters) return messages; - var lastMessageIndex = -1; - var lastTextIndex = -1; - for (var i = messages.Count - 1; i >= 0 && lastMessageIndex < 0; i--) + var marker = TruncationSuffix[..Math.Min(TruncationSuffix.Length, maxCharacters)]; + var remaining = maxCharacters - marker.Length; + var truncated = false; + return messages.Select(message => { - var contents = messages[i].Contents; - for (var j = contents.Count - 1; j >= 0; j--) + var contents = message.Contents.Select(content => { - if (contents[j] is not TextContent) continue; - lastMessageIndex = i; - lastTextIndex = j; - break; - } - } - - // No TextContent anywhere in the response — nothing this function is responsible for can trim. - if (lastMessageIndex < 0) - return messages; - - var lastText = (TextContent)messages[lastMessageIndex].Contents[lastTextIndex]; - var budget = Math.Max(0, maxCharacters - (totalLength - lastText.Text.Length)); - var truncatedText = (budget < lastText.Text.Length ? lastText.Text[..budget] : lastText.Text) + TruncationSuffix; + if (content is not TextContent text) return content; + if (truncated) return new TextContent(""); + if (text.Text.Length <= remaining) + { + remaining -= text.Text.Length; + return content; + } - var newContents = messages[lastMessageIndex].Contents.ToList(); - newContents[lastTextIndex] = new TextContent(truncatedText); - var newMessages = messages.ToList(); - newMessages[lastMessageIndex] = CloneWithContents(messages[lastMessageIndex], newContents); - return newMessages; + truncated = true; + return new TextContent(text.Text[..remaining] + marker); + }).ToList(); + return CloneWithContents(message, contents); + }).ToList(); } // Core mapper: rewrites only TextContent items (via `transform`) in a message's Contents, diff --git a/GuardRails.AgentFramework/Program.cs b/GuardRails.AgentFramework/Program.cs index fe85e58..73ca64d 100644 --- a/GuardRails.AgentFramework/Program.cs +++ b/GuardRails.AgentFramework/Program.cs @@ -78,8 +78,8 @@ async Task OutputGuardMiddleware( { var response = await innerAgent.RunAsync(messages, session, options, cancellationToken); - // Truncate on total text length, but only rewrite the last TextContent — function calls, - // function results and earlier text stay intact instead of being flattened into one message. + // Keep the response prefix within the total text budget. Function calls and function results + // stay intact instead of being flattened into one message. var truncatedMessages = GuardRails.TruncateMessages(response.Messages, 2000); if (!ReferenceEquals(truncatedMessages, response.Messages)) { diff --git a/MemoryConsolidation.AgentFramework/EpisodicStore.cs b/MemoryConsolidation.AgentFramework/EpisodicStore.cs index 8def473..8b242da 100644 --- a/MemoryConsolidation.AgentFramework/EpisodicStore.cs +++ b/MemoryConsolidation.AgentFramework/EpisodicStore.cs @@ -28,7 +28,7 @@ public sealed record Scored(Episode Episode, double Recency, double Relevance, d /// the rare significant event retrievable long after it stops being recent. public static class EpisodicRetrieval { - /// Half-life in hours: a memory a day old counts about a fifth of a fresh one. + /// Per-hour retention: a day-old memory retains about 89%; half-life is about 5.8 days. const double DecayPerHour = 0.995; /// Scores the ACTIVE episodes only. Archived ones are still on disk and still auditable; they diff --git a/PatternExplorer/patterns/MemoryConsolidation.md b/PatternExplorer/patterns/MemoryConsolidation.md index ee05801..56a6f39 100644 --- a/PatternExplorer/patterns/MemoryConsolidation.md +++ b/PatternExplorer/patterns/MemoryConsolidation.md @@ -48,8 +48,8 @@ Eight episodes across three topics span 45 days, each with an importance scored the host here; usually a cheap model call in production). **Retrieval.** `EpisodicRetrieval.Score` computes `recency + importance + relevance` for a query -about export timeouts. Recency is exponential decay at 0.995 per hour — a day-old memory counts -about a fifth of a fresh one. Relevance is word overlap, with a `ponytail:` note that a real +about export timeouts. Recency is exponential decay at 0.995 per hour — a day-old memory retains +about 89%, and the half-life is about 5.8 days. Relevance is word overlap, with a `ponytail:` note that a real system swaps in the embedding generator from the **RAG** sample; the scoring formula around it does not change. diff --git a/RegressionEvals.AgentFramework/CasePartition.cs b/RegressionEvals.AgentFramework/CasePartition.cs index 6846206..b9693f6 100644 --- a/RegressionEvals.AgentFramework/CasePartition.cs +++ b/RegressionEvals.AgentFramework/CasePartition.cs @@ -13,7 +13,7 @@ public static (IReadOnlyList Evaluated, IReadOnlyList Aw var evaluated = new List(); var awaitingReview = new List(); foreach (var c in cases) - (string.IsNullOrEmpty(c.ReviewedBy) ? awaitingReview : evaluated).Add(c); + (string.IsNullOrWhiteSpace(c.ReviewedBy) ? awaitingReview : evaluated).Add(c); return (evaluated, awaitingReview); }