diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fb5bb1d5..a0c107ee 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -110,6 +110,7 @@ jobs: dotnet build samples/AgentMemory.Sample.MinimalAgent/AgentMemory.Sample.MinimalAgent.csproj -c Release --no-restore dotnet build samples/AgentMemory.Sample.BlendedAgent/AgentMemory.Sample.BlendedAgent.csproj -c Release --no-restore dotnet build samples/AgentMemory.Sample.McpHost/AgentMemory.Sample.McpHost.csproj -c Release --no-restore + dotnet build tools/AgentMemory.McpHost/AgentMemory.McpHost.csproj -c Release --no-restore dotnet build samples/AgentMemory.Sample.ShoppingAssistant/AgentMemory.Sample.ShoppingAssistant.csproj -c Release --no-restore dotnet build samples/AgentMemory.Sample.NamsAgent/AgentMemory.Sample.NamsAgent.csproj -c Release --no-restore dotnet build samples/AspireDemo/AspireDemo.AppHost/AspireDemo.AppHost.csproj -c Release --no-restore diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 89fff5b9..dd5b0d65 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -115,6 +115,7 @@ jobs: dotnet build samples/AgentMemory.Sample.MinimalAgent/AgentMemory.Sample.MinimalAgent.csproj -c Release --no-restore dotnet build samples/AgentMemory.Sample.BlendedAgent/AgentMemory.Sample.BlendedAgent.csproj -c Release --no-restore dotnet build samples/AgentMemory.Sample.McpHost/AgentMemory.Sample.McpHost.csproj -c Release --no-restore + dotnet build tools/AgentMemory.McpHost/AgentMemory.McpHost.csproj -c Release --no-restore dotnet build samples/AgentMemory.Sample.ShoppingAssistant/AgentMemory.Sample.ShoppingAssistant.csproj -c Release --no-restore dotnet build samples/AspireDemo/AspireDemo.AppHost/AspireDemo.AppHost.csproj -c Release --no-restore dotnet build samples/AspireDemo/AspireDemo.DemoApp/AspireDemo.DemoApp.csproj -c Release --no-restore diff --git a/AgentMemory.slnx b/AgentMemory.slnx index afe8c063..d08aa504 100644 --- a/AgentMemory.slnx +++ b/AgentMemory.slnx @@ -33,6 +33,7 @@ + diff --git a/CHANGELOG.md b/CHANGELOG.md index 211ad02f..7310c9a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,22 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Procedural memory: a reasoning trace can be promoted to a reusable procedure.** `TraceKind` + (`Episode` by default) marks a trace as a procedure; `trace_kind_idx` makes it seekable; and + task-similarity search takes an opt-in `proceduresOnly` filter, **null by default** so existing + Cypher is byte-identical. + + A trace and a procedure are the same record read two ways: an episode says what happened *once*, + a procedure says what to do *next time* — they differ by retrieval key. + + **The load-bearing part is the retention exemption.** `PruneSessionTraces` orders by `started_at` + with age as its *only* criterion and fires on every trace creation once `MaxTracesPerSession` is + set — so without it a promoted procedure is deleted by recency and the capability does not exist. + The exemption is NULL-safe in both directions: a trace written before `trace_kind` existed is still + prunable (or a retention cap silently stops capping) and still visible to an episode filter. + + New migration `0011_trace_kind.cypher` brings existing databases to parity. + - **Live recall can now honour a fact's valid-time window** — `RecallOptions.ValidTime` (`Ignore` by default, so nothing changes unless you ask). diff --git a/Directory.Build.props b/Directory.Build.props index 6dda5b35..79aa878e 100644 --- a/Directory.Build.props +++ b/Directory.Build.props @@ -21,8 +21,10 @@ still-widely-deployed .NET 8 LTS use the library without adopting a newer runtime; net10.0 keeps pace with the newest release. Verified with real builds and executed tests on all three TFMs, not just compiled. Scoped the same way as the packaging metadata below, plus excluding the four - non-packable tools/ console apps (Cli, LongMemEval, TckBridge, TckBridge.Nams), which stay single-targeted. --> - + non-packable tools/ console apps (Cli, LongMemEval, TckBridge, TckBridge.Nams) and the McpHost global tool, which stay + single-targeted: a DotnetTool package resolves one framework at install time, so multi-targeting + it only inflates the package. --> + net10.0;net9.0;net8.0 diff --git a/docs/architecture.md b/docs/architecture.md index fe84a099..c9407fb9 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -142,7 +142,7 @@ graph TD | **Purpose** | Domain contracts — all models, interfaces, and configuration types shared across the system | | **Dependencies** | **Microsoft.Extensions.AI.Abstractions** 10.8.0 (approved, D-AR2-1) — .NET BCL otherwise (multi-targets net8.0/net9.0/net10.0) | | **MUST NOT reference** | Neo4j.Driver, Microsoft.Agents.*, any GraphRAG SDK, any MCP SDK, any NuGet package **except** Microsoft.Extensions.AI.Abstractions | -| **Key types** | 52 domain records (Conversation, Message, Entity, Fact, Preference, Relationship, MemoryHistoryQuery, MemoryHistoryRecord, ReasoningTrace, ReasoningStep, ToolCall, ToolCallStats, IngestionItemOutcome, MemoryContextRankedItem, MemoryContextSectionDiagnostics, UnifiedExtractionResult, etc.), 41 service interfaces (incl. `IMemoryIsolationPolicy`, `IUnifiedMemoryExtractor`, and `IMultiSessionUnifiedMemoryExtractor`), 11 repository interfaces, 16 configuration types (incl. `MemoryRankingOptions`, `MemoryIsolationOptions`), 26 enums (incl. `MemoryProfile`, `RankingIntent`, `DuplicateStatus`, `EntityMatchType`, `MemoryNodeKind`, `MemoryOperationAccess`, `MemoryIsolationMode`, `IngestionStatus`, `IngestionStage`, `IngestionItemStatus`, `MemoryItemKind`, `IngestionFailureMode`, `MemoryTrustLevel`, `AssistantContentMode`, `TemporalValidityMode`) | +| **Key types** | 52 domain records (Conversation, Message, Entity, Fact, Preference, Relationship, MemoryHistoryQuery, MemoryHistoryRecord, ReasoningTrace, ReasoningStep, ToolCall, ToolCallStats, IngestionItemOutcome, MemoryContextRankedItem, MemoryContextSectionDiagnostics, UnifiedExtractionResult, etc.), 41 service interfaces (incl. `IMemoryIsolationPolicy`, `IUnifiedMemoryExtractor`, and `IMultiSessionUnifiedMemoryExtractor`), 11 repository interfaces, 16 configuration types (incl. `MemoryRankingOptions`, `MemoryIsolationOptions`), 26 enums (incl. `MemoryProfile`, `RankingIntent`, `DuplicateStatus`, `EntityMatchType`, `MemoryNodeKind`, `MemoryOperationAccess`, `MemoryIsolationMode`, `IngestionStatus`, `IngestionStage`, `IngestionItemStatus`, `MemoryItemKind`, `IngestionFailureMode`, `MemoryTrustLevel`, `AssistantContentMode`, `TemporalValidityMode`, `TraceKind`, `ExtractionProvenanceMode`) | **Namespace structure:** ``` diff --git a/eng/release-packages.txt b/eng/release-packages.txt index 04f95ab6..c4c217d6 100644 --- a/eng/release-packages.txt +++ b/eng/release-packages.txt @@ -11,6 +11,10 @@ AgentMemory.Enrichment|src/AgentMemory.Enrichment/AgentMemory.Enrichment.csproj AgentMemory.Extraction.AzureLanguage|src/AgentMemory.Extraction.AzureLanguage/AgentMemory.Extraction.AzureLanguage.csproj AgentMemory.Extraction.Llm|src/AgentMemory.Extraction.Llm/AgentMemory.Extraction.Llm.csproj AgentMemory.McpServer|src/AgentMemory.McpServer/AgentMemory.McpServer.csproj +# The one packable project outside src/: a DotnetTool, not a library. It is not referenced by any +# eng/package-consumers/ project because a tool package cannot be a PackageReference -- it is +# installed, not consumed -- so the consumer-install verification does not and should not cover it. +AgentMemory.McpHost|tools/AgentMemory.McpHost/AgentMemory.McpHost.csproj AgentMemory.McpServer.Nams|src/AgentMemory.McpServer.Nams/AgentMemory.McpServer.Nams.csproj AgentMemory.Nams|src/AgentMemory.Nams/AgentMemory.Nams.csproj AgentMemory.Neo4j|src/AgentMemory.Neo4j/AgentMemory.Neo4j.csproj diff --git a/src/AgentMemory.Abstractions/Domain/Extraction/ExtractedFact.cs b/src/AgentMemory.Abstractions/Domain/Extraction/ExtractedFact.cs index c6c7c69d..a6257ee1 100644 --- a/src/AgentMemory.Abstractions/Domain/Extraction/ExtractedFact.cs +++ b/src/AgentMemory.Abstractions/Domain/Extraction/ExtractedFact.cs @@ -34,4 +34,47 @@ public sealed record ExtractedFact /// Optional end of validity period. /// public DateTimeOffset? ValidUntil { get; init; } + + /// + /// The conversational role of the turn this fact was derived from ("user", + /// "assistant", …), or when the extractor did not report one. + /// + /// + /// + /// Trust is otherwise stamped once per extraction request and applied to every item in the + /// batch, so a batch containing both a user's statement and a claim the model itself made records + /// them identically. That is tolerable only while assistant content is not extracted at all — which + /// is the shipped default (AssistantContentMode.Ignore) — and stops being tolerable the + /// moment it is switched on, because the enum's central distinction between a user's claim and the + /// model's own would be lost at exactly the point it first carries weight. + /// + /// + /// Null is the meaningful value, and it means "unchanged". Extractors populate this only when + /// assistant content is being extracted, so at defaults it is null everywhere and persistence + /// applies the request's trust level exactly as it always did. It is a self-report by the model + /// rather than a derived fact — a per-item source binding would need per-item provenance, which the + /// batch-level EXTRACTED_FROM edge does not yet carry — so it may only refine a trust + /// stamp, never relax the guarantees around it. + /// + /// + public string? SourceRole { get; init; } + + /// + /// The 1-based turn number this fact was stated in, or when the extractor + /// did not report one. + /// + /// + /// + /// Populated only under , which numbers the + /// turns in the extraction transcript and asks which one stated each item. It resolves to a single + /// source message, replacing the batch-level link in which a fact points at a mean of 12 messages + /// and as many as 30 — a breadth that makes any attribution metric derived from the edge true by + /// construction. + /// + /// + /// Out of range or absent falls back to the batch links. Coarse provenance is recoverable; missing + /// provenance is not, and a hallucinated turn number must not be able to erase the real answer. + /// + /// + public int? SourceTurn { get; init; } } diff --git a/src/AgentMemory.Abstractions/Domain/Extraction/ExtractedPreference.cs b/src/AgentMemory.Abstractions/Domain/Extraction/ExtractedPreference.cs index 3346d038..2c669c0e 100644 --- a/src/AgentMemory.Abstractions/Domain/Extraction/ExtractedPreference.cs +++ b/src/AgentMemory.Abstractions/Domain/Extraction/ExtractedPreference.cs @@ -24,4 +24,15 @@ public sealed record ExtractedPreference /// Confidence score (0.0 to 1.0). /// public double Confidence { get; init; } = 1.0; + + /// + /// + /// Preferences carry this for the same reason facts do, and arguably a stronger one: a preference + /// the assistant attributed to the user ("you seem to prefer …") becomes a durable statement + /// about that user, and is indistinguishable after the fact from one the user actually stated. + /// + public string? SourceRole { get; init; } + + /// + public int? SourceTurn { get; init; } } diff --git a/src/AgentMemory.Abstractions/Domain/Reasoning/ReasoningTrace.cs b/src/AgentMemory.Abstractions/Domain/Reasoning/ReasoningTrace.cs index 21db6ec6..595142c2 100644 --- a/src/AgentMemory.Abstractions/Domain/Reasoning/ReasoningTrace.cs +++ b/src/AgentMemory.Abstractions/Domain/Reasoning/ReasoningTrace.cs @@ -1,4 +1,4 @@ -namespace AgentMemory.Abstractions.Domain; +namespace AgentMemory.Abstractions.Domain; /// /// Represents a reasoning trace for a task or agent run. @@ -51,6 +51,15 @@ public sealed record ReasoningTrace /// public string? OwnerId { get; init; } + /// + /// Whether this trace is an ordinary episode or a promoted, reusable procedure. + /// + /// + /// Defaults to , which is what every existing trace is, so nothing + /// changes for a store written before this existed. + /// + public TraceKind Kind { get; init; } = TraceKind.Episode; + /// /// Additional metadata. /// diff --git a/src/AgentMemory.Abstractions/Domain/Reasoning/TraceKind.cs b/src/AgentMemory.Abstractions/Domain/Reasoning/TraceKind.cs new file mode 100644 index 00000000..671ac636 --- /dev/null +++ b/src/AgentMemory.Abstractions/Domain/Reasoning/TraceKind.cs @@ -0,0 +1,37 @@ +namespace AgentMemory.Abstractions.Domain; + +/// +/// What a stored is for. +/// +/// +/// +/// A trace and a procedure are the same underlying record read two ways. An episode says what +/// happened once — retrieved by when it happened, read as a claim. A procedure says what to do +/// next time — retrieved by similarity of the task, and replayed as steps. Same incident, +/// different retrieval key, which is what makes them different kinds rather than one kind named twice. +/// +/// +/// Named trace_kind on the node, never kind. kind already means +/// "audit-node discriminator" both here and upstream, and overloading a property whose meaning is +/// shared with another implementation is the changed-semantics hazard that a schema-parity check +/// exists to catch. A new property is ungated by the parity verifier; a new label or edge is not. +/// +/// +public enum TraceKind +{ + /// + /// An ordinary recorded episode. The default, and what every existing trace is. + /// + Episode = 0, + + /// + /// A trace promoted to a reusable procedure: retrievable by task similarity and exempt from + /// recency-based retention pruning. + /// + /// + /// The exemption is not a nicety. PruneSessionTraces orders by started_at with age as + /// its only criterion and fires on every trace creation once a per-session cap is set — so + /// without it, promotion is silently undone by recency and the capability does not exist. + /// + Procedure = 1, +} diff --git a/src/AgentMemory.Abstractions/Options/ExtractionOptions.cs b/src/AgentMemory.Abstractions/Options/ExtractionOptions.cs index 0529cbc3..535dc33a 100644 --- a/src/AgentMemory.Abstractions/Options/ExtractionOptions.cs +++ b/src/AgentMemory.Abstractions/Options/ExtractionOptions.cs @@ -71,6 +71,30 @@ public sealed class ExtractionOptions /// . /// public MemoryTrustLevel DefaultTrustLevel { get; set; } = MemoryTrustLevel.UserProvided; + + /// + /// Whether a newly written fact about a functional relation supersedes the earlier + /// assertion it replaces, instead of accumulating beside it (M1). + /// + /// + /// + /// Off by default. It changes what live recall returns — a superseded fact drops out of it — and + /// every recorded measurement was taken with append-only writes, so a default flip would move + /// results with no setting changed. + /// + /// + /// Only relations the vocabulary declares functional are eligible (lives in, + /// works at, …). A person likes many things and attends many events; superseding a + /// multi-valued predicate would close a true fact. Undeclared predicates, including any the + /// extractor invents, are treated as multi-valued and are never superseded. + /// + /// + /// Non-destructive: losers keep their content, gain invalidated_at and a + /// :SUPERSEDED_BY edge, and stay visible to as-of recall. Requires a store implementing + /// IFactRepository.FindSupersededCandidatesAsync; one that does not simply keeps appending. + /// + /// + public bool SupersedeReplacedFacts { get; set; } } /// Controls which matching strategies are used for entity resolution. diff --git a/src/AgentMemory.Abstractions/Options/ExtractionProvenanceMode.cs b/src/AgentMemory.Abstractions/Options/ExtractionProvenanceMode.cs new file mode 100644 index 00000000..c33828e0 --- /dev/null +++ b/src/AgentMemory.Abstractions/Options/ExtractionProvenanceMode.cs @@ -0,0 +1,42 @@ +namespace AgentMemory.Abstractions.Options; + +/// +/// How precisely a stored memory is bound to the conversation turn it came from. +/// +/// +/// +/// EXTRACTED_FROM is written per ingestion batch: every item extracted from a call is +/// linked to every message that call saw. Measured on the evaluation corpus, a single fact links to a +/// mean of 12 source messages and as many as 30. That is broader than the field's other +/// implementations — upstream binds a mention to one message with character offsets, Zep binds to one +/// episode — and being broader and coarser is the worst of both, because any attribution +/// metric derived from that edge is satisfied by construction and can never fail: ask "is the +/// source of this fact among its linked messages?" and the answer is yes for a batch of thirty. +/// +/// +/// This is opt-in and defaults to for one reason: numbers the +/// turns in the extraction transcript and asks the model which one stated each item, so it changes the +/// prompt and the rendered conversation. Prompt bytes are fingerprinted into every measured run +/// here, and a default that moved them would silently invalidate every sealed base. +/// +/// +public enum ExtractionProvenanceMode +{ + /// + /// Link every extracted item to every message the extraction call saw. The behaviour that shipped, + /// and the one every recorded measurement was taken under. + /// + Batch = 0, + + /// + /// Ask the model which turn stated each fact and preference, and link only that message. + /// + /// + /// Applies to facts and preferences, not entities — deliberately. A fact asserts one claim made in + /// one statement, so binding it to thirty messages is a loss of information. An Entity node + /// is a merged identity that legitimately appears across many turns, so narrowing it to a + /// single turn would be wrong rather than precise. An unreported or out-of-range turn falls back to + /// the batch links: coarse provenance is recoverable, missing provenance is not. + /// + PerItem = 1, +} diff --git a/src/AgentMemory.Abstractions/Repositories/IFactRepository.cs b/src/AgentMemory.Abstractions/Repositories/IFactRepository.cs index 9ec32a43..48408b58 100644 --- a/src/AgentMemory.Abstractions/Repositories/IFactRepository.cs +++ b/src/AgentMemory.Abstractions/Repositories/IFactRepository.cs @@ -118,6 +118,33 @@ public interface IFactRepository /// Task SupersedeAsync(string loserFactId, string winnerFactId, MemoryScope? scope = null, CancellationToken cancellationToken = default); + /// + /// The live facts asserting a different object for the same subject and predicate as + /// — the ones a newly written fact about a functional relation + /// replaces (M1 write-time supersession). Never returns the winner itself. + /// + /// + /// + /// Matching is on the canonical subject_key/predicate_key/object_key, the same + /// keys the write path MERGEs on, so a restatement in different words is recognised as the same + /// assertion rather than accumulating beside it. + /// + /// + /// Default: none. A store that has not implemented this simply does not perform write-time + /// supersession — the append behaviour it already had. Returning nothing is the only safe default: + /// throwing would break every third-party repository on a feature they never opted into, and there + /// is no store-agnostic way to answer the question correctly. + /// + /// + Task> FindSupersededCandidatesAsync( + string winnerFactId, + string subject, + string predicate, + string @object, + MemoryScope? scope = null, + CancellationToken cancellationToken = default) => + Task.FromResult>([]); + /// /// Finds an existing fact matching the subject-predicate-object triple. When /// is supplied (R1) the lookup is confined to the owner's own and (optionally) shared facts. Null diff --git a/src/AgentMemory.Abstractions/Repositories/IReasoningTraceRepository.cs b/src/AgentMemory.Abstractions/Repositories/IReasoningTraceRepository.cs index 7e4f855e..e67bf8a6 100644 --- a/src/AgentMemory.Abstractions/Repositories/IReasoningTraceRepository.cs +++ b/src/AgentMemory.Abstractions/Repositories/IReasoningTraceRepository.cs @@ -1,4 +1,4 @@ -using AgentMemory.Abstractions.Domain; +using AgentMemory.Abstractions.Domain; using AgentMemory.Abstractions.Options; namespace AgentMemory.Abstractions.Repositories; @@ -48,7 +48,26 @@ Task> ListAllAsync( CancellationToken cancellationToken = default); /// - /// Point-in-time variant of : only traces that had started at + /// Task-similarity search restricted to (or excluding) promoted procedures. + /// + /// + /// A default interface method, not a new parameter: the surface is locked under SemVer. The default + /// ignores and calls the overload above, which is what every + /// implementation does today — a store with no promotion concept keeps working and simply does not + /// filter, rather than appearing to. + /// + Task> SearchByTaskVectorAsync( + float[] taskEmbedding, + bool? proceduresOnly, + bool? successFilter = null, + int limit = 10, + double minScore = 0.0, + MemoryScope? scope = null, + CancellationToken cancellationToken = default) => + SearchByTaskVectorAsync(taskEmbedding, successFilter, limit, minScore, scope, cancellationToken); + + /// + /// Point-in-time variant of SearchByTaskVectorAsync: only traces that had started at /// or before , optionally scoped to an owner (R1). /// Task> SearchByTaskVectorAsOfAsync( diff --git a/src/AgentMemory.Core/Extraction/ConversationTextBuilder.cs b/src/AgentMemory.Core/Extraction/ConversationTextBuilder.cs index 68c480e6..6d43797d 100644 --- a/src/AgentMemory.Core/Extraction/ConversationTextBuilder.cs +++ b/src/AgentMemory.Core/Extraction/ConversationTextBuilder.cs @@ -1,3 +1,5 @@ +using System.Globalization; +using System.Text; using AgentMemory.Abstractions.Domain; namespace AgentMemory.Core.Extraction; @@ -16,4 +18,36 @@ internal static class ConversationTextBuilder /// A single string containing one line per message in the form Role: Content. public static string Build(IReadOnlyList messages) => string.Join("\n", messages.Select(m => $"{m.Role}: {m.Content}")); + + /// + /// Builds the transcript with each turn numbered from 1, as [N] Role: Content. + /// + /// + /// + /// The numbering is what makes a per-item provenance answer expressible: without it the model has + /// no way to name a turn, and EXTRACTED_FROM can only be written for the whole batch. + /// + /// + /// 1-based, and positional. Turn N is messages[N-1], which is the same order + /// the caller derives its source-message ids in, so resolution is a direct index rather than a + /// lookup that could silently mismatch. Kept as a separate method rather than a flag on + /// so the unnumbered rendering — the one every recorded + /// measurement used — cannot change by accident. + /// + /// + public static string BuildNumbered(IReadOnlyList messages) + { + var builder = new StringBuilder(); + for (var index = 0; index < messages.Count; index++) + { + if (index > 0) builder.Append('\n'); + builder.Append('[') + .Append((index + 1).ToString(CultureInfo.InvariantCulture)) + .Append("] ") + .Append(messages[index].Role) + .Append(": ") + .Append(messages[index].Content); + } + return builder.ToString(); + } } diff --git a/src/AgentMemory.Core/Extraction/PersistenceStage.cs b/src/AgentMemory.Core/Extraction/PersistenceStage.cs index b085e800..a5f3834d 100644 --- a/src/AgentMemory.Core/Extraction/PersistenceStage.cs +++ b/src/AgentMemory.Core/Extraction/PersistenceStage.cs @@ -257,9 +257,18 @@ async Task PersistEntityIndividuallyAsync(string name, Entity item) : await _factRepository.FindByTripleAsync( extracted.Subject, extracted.Predicate, extracted.Object, MemoryScope.For(ownerId, includeShared: false), cancellationToken).ConfigureAwait(false); + // Per-item refinement before the existing per-batch composition. At defaults SourceRole + // is null on every item and this is the identity, so the trust a host sees is byte-for- + // byte what it was; it becomes non-trivial only when assistant content is extracted, + // which is the exact moment "who claimed this" stops being answerable from the batch. + var requestTrustLevel = SourceRoleTrust.Refine(trustLevel, extracted.SourceRole); + // Per-item provenance (L3c). Identity at defaults -- SourceTurn is null unless + // ExtractionProvenanceMode.PerItem asked for it -- and a reported turn that does not + // resolve keeps the batch links rather than inventing a narrower wrong one. + var factMessageIds = SourceTurnProvenance.Resolve(extracted.SourceTurn, sourceMessageIds); var effectiveFactTrustLevel = existingFact is null - ? trustLevel - : MaxTrustLevel(existingFact.Metadata.GetTrustLevel(), trustLevel); + ? requestTrustLevel + : MaxTrustLevel(existingFact.Metadata.GetTrustLevel(), requestTrustLevel); var factMetadata = existingFact is null ? MemoryTrustMetadataExtensions.CreateWithTrustLevel(effectiveFactTrustLevel) : existingFact.Metadata.WithTrustLevel(effectiveFactTrustLevel); @@ -275,7 +284,7 @@ async Task PersistEntityIndividuallyAsync(string name, Entity item) ValidUntil = extracted.ValidUntil, Embedding = preparedFact.Embedding, OwnerId = ownerId, - SourceMessageIds = sourceMessageIds, + SourceMessageIds = factMessageIds, CreatedAtUtc = _clock.UtcNow, Metadata = factMetadata }, factSourceKey); @@ -291,13 +300,18 @@ async Task PersistEntityIndividuallyAsync(string name, Entity item) } } - async Task RecordPersistedFactAsync(string sourceKey, Fact persisted) + async Task RecordPersistedFactAsync( + string sourceKey, Fact persisted, IReadOnlyList provenanceMessageIds) { // Fact upsert MERGEs on the natural triple and may return an older stable id. Always use // the repository result for outcomes and provenance rather than the fresh caller id. RecordSuccess(outcomes, MemoryItemKind.Fact, sourceKey, persisted.FactId); - foreach (var msgId in ExplicitProvenanceMessageIds(_factRepository, sourceMessageIds)) + // The INPUT item's resolved ids, not the persisted result's: a MERGE returns the stored + // node, whose source ids may be the union accumulated over earlier ingestions. Writing + // edges from that would re-link this fact to messages it was not extracted from now -- + // which is exactly the batch-level breadth per-item provenance exists to remove. + foreach (var msgId in ExplicitProvenanceMessageIds(_factRepository, provenanceMessageIds)) { try { @@ -316,17 +330,54 @@ await _factRepository.CreateExtractedFromRelationshipAsync( } } + await SupersedeReplacedFactsAsync(persisted).ConfigureAwait(false); + persistedFactCount++; _logger.LogDebug("Persisted fact '{S} {P} {O}'.", persisted.Subject, persisted.Predicate, persisted.Object); } + // M1 write-time UPDATE. Runs after the write, so the incoming fact is already the winner and a + // failure here leaves the graph in the append-only state it was in before -- strictly the old + // behaviour, never a half-resolved one. Best-effort by design: losing a supersession costs + // precision in live recall, while failing the ingestion over it would lose the memory itself. + async Task SupersedeReplacedFactsAsync(Fact winner) + { + if (!_options.SupersedeReplacedFacts || !WriteTimeFactResolution.CanSupersede(winner)) + return; + + var scope = string.IsNullOrEmpty(ownerId) ? null : MemoryScope.For(ownerId, includeShared: false); + try + { + var losers = await _factRepository.FindSupersededCandidatesAsync( + winner.FactId, winner.Subject, winner.Predicate, winner.Object, scope, + cancellationToken).ConfigureAwait(false); + + foreach (var loser in losers) + { + await _factRepository.SupersedeAsync( + loser.FactId, winner.FactId, scope, cancellationToken).ConfigureAwait(false); + _logger.LogDebug( + "Superseded fact '{Loser}' with '{Winner}' ({S} {P}).", + loser.FactId, winner.FactId, winner.Subject, winner.Predicate); + } + } + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { throw; } + catch (Exception ex) + { + _logger.LogWarning(ex, + "Write-time supersession failed for fact '{Id}'; it remains stored alongside the " + + "assertion it replaces.", winner.FactId); + } + } + async Task PersistFactIndividuallyAsync(Fact item, string sourceKey) { try { var persisted = await _factRepository.UpsertAsync(item, cancellationToken).ConfigureAwait(false); - await RecordPersistedFactAsync(sourceKey, persisted).ConfigureAwait(false); + await RecordPersistedFactAsync( + sourceKey, persisted, item.SourceMessageIds).ConfigureAwait(false); } catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { throw; } catch (MemoryIngestionException) { throw; } @@ -390,7 +441,8 @@ async Task PersistFactIndividuallyAsync(Fact item, string sourceKey) { foreach (var input in factInputs) await RecordPersistedFactAsync( - input.SourceKey, batchedFactsByKey[FactKey(input.Item)]).ConfigureAwait(false); + input.SourceKey, batchedFactsByKey[FactKey(input.Item)], + input.Item.SourceMessageIds).ConfigureAwait(false); } else { @@ -422,19 +474,22 @@ await RecordPersistedFactAsync( Confidence = extracted.Confidence, Embedding = preparedPreference.Embedding, OwnerId = ownerId, - SourceMessageIds = sourceMessageIds, + SourceMessageIds = SourceTurnProvenance.Resolve(extracted.SourceTurn, sourceMessageIds), CreatedAtUtc = _clock.UtcNow, - Metadata = MemoryTrustMetadataExtensions.CreateWithTrustLevel(trustLevel) + Metadata = MemoryTrustMetadataExtensions.CreateWithTrustLevel( + SourceRoleTrust.Refine(trustLevel, extracted.SourceRole)) }, SourceKey: extracted.PreferenceText); }).ToList(); var persistedPrefCount = 0; - async Task RecordPersistedPreferenceAsync(string sourceKey, Preference persisted) + async Task RecordPersistedPreferenceAsync( + string sourceKey, Preference persisted, IReadOnlyList provenanceMessageIds) { RecordSuccess(outcomes, MemoryItemKind.Preference, sourceKey, persisted.PreferenceId); - foreach (var msgId in ExplicitProvenanceMessageIds(_preferenceRepository, sourceMessageIds)) + // The input item's resolved ids, for the same reason the fact path uses them. + foreach (var msgId in ExplicitProvenanceMessageIds(_preferenceRepository, provenanceMessageIds)) { try { @@ -462,7 +517,8 @@ async Task PersistPreferenceIndividuallyAsync(Preference item, string sourceKey) try { var persisted = await _preferenceRepository.UpsertAsync(item, cancellationToken).ConfigureAwait(false); - await RecordPersistedPreferenceAsync(sourceKey, persisted).ConfigureAwait(false); + await RecordPersistedPreferenceAsync( + sourceKey, persisted, item.SourceMessageIds).ConfigureAwait(false); } catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { throw; } catch (MemoryIngestionException) { throw; } @@ -511,7 +567,8 @@ async Task PersistPreferenceIndividuallyAsync(Preference item, string sourceKey) { foreach (var input in preferenceInputs) await RecordPersistedPreferenceAsync( - input.SourceKey, batchedPreferencesById[input.Item.PreferenceId]).ConfigureAwait(false); + input.SourceKey, batchedPreferencesById[input.Item.PreferenceId], + input.Item.SourceMessageIds).ConfigureAwait(false); } else { diff --git a/src/AgentMemory.Core/Extraction/SourceRoleTrust.cs b/src/AgentMemory.Core/Extraction/SourceRoleTrust.cs new file mode 100644 index 00000000..de416f75 --- /dev/null +++ b/src/AgentMemory.Core/Extraction/SourceRoleTrust.cs @@ -0,0 +1,59 @@ +using AgentMemory.Abstractions.Domain; + +namespace AgentMemory.Core.Extraction; + +/// +/// Maps a reported source-turn role onto the trust level that turn's content deserves. +/// +/// +/// +/// One mapping, in one place, so the meaning of "this came from the assistant" cannot differ between +/// facts and preferences, or between the three extractors. The same mapping already exists correctly +/// in the NAMS subsystem ("assistant" => ModelGenerated); this is the extraction pipeline +/// finally being able to express it. +/// +/// +/// Only assistant is mapped, deliberately. The obvious extension — user → +/// , tool → +/// — would be wrong here. is ordered so a numeric >= means "at +/// least this trusted", and the default request trust is , so +/// those mappings would raise trust on hosts that never asked for any, purely on the strength of +/// a label the model wrote about itself. Admission bypass and the system-role gate both compare with +/// >=, so that is a security-relevant direction, not a cosmetic one. +/// +/// +/// The assistant case is the one exception because it is the case this exists to record, it is +/// requested only when assistant content is actually being extracted, and mislabelling in the other +/// direction — a model-generated claim recorded as if a user had said it — is precisely the failure +/// mode being closed. A null return means "say nothing", which leaves the request's own trust level +/// applying unchanged. +/// +/// +internal static class SourceRoleTrust +{ + /// The role name extractors report for a model turn. + internal const string AssistantRole = "assistant"; + + /// + /// The trust level implied by , or when the + /// role is absent, unrecognised, or one this mapping deliberately declines to interpret. + /// + internal static MemoryTrustLevel? FromSourceRole(string? sourceRole) => + string.Equals(sourceRole, AssistantRole, StringComparison.OrdinalIgnoreCase) + ? MemoryTrustLevel.ModelGenerated + : null; + + /// + /// Combines the request-level trust with whatever the reported role implies, taking the higher of + /// the two. + /// + /// + /// Max, not override, so this composes with the existing monotonic rule rather than competing with + /// it: a host that declared the whole ingestion + /// is making a statement about the ingestion, and a model's self-report must not quietly demote it. + /// + internal static MemoryTrustLevel Refine(MemoryTrustLevel requestTrustLevel, string? sourceRole) => + FromSourceRole(sourceRole) is { } implied && implied > requestTrustLevel + ? implied + : requestTrustLevel; +} diff --git a/src/AgentMemory.Core/Extraction/SourceTurnProvenance.cs b/src/AgentMemory.Core/Extraction/SourceTurnProvenance.cs new file mode 100644 index 00000000..006e021e --- /dev/null +++ b/src/AgentMemory.Core/Extraction/SourceTurnProvenance.cs @@ -0,0 +1,47 @@ +namespace AgentMemory.Core.Extraction; + +/// +/// Resolves a reported 1-based turn number to the single source message it names. +/// +/// +/// +/// EXTRACTED_FROM is written per ingestion batch: every item is linked to every message the +/// extraction call saw. On the evaluation corpus a fact links to a mean of 12 source messages +/// and as many as 30, which is broad enough that any attribution metric derived from that edge is +/// satisfied by construction — "is the true source among the linked messages?" is trivially yes +/// across thirty of them, so the metric can never fail and therefore measures nothing. +/// +/// +/// The turn number is positional by design. The transcript is rendered from the same ordered message +/// list the source-message ids are derived from, so turn N is index N-1 — a direct index +/// rather than a lookup that could silently mismatch. +/// +/// +/// Falls back rather than failing. An absent, zero, negative or out-of-range turn keeps the +/// batch links. A model that reports a turn number for a conversation of five as 12 has told us +/// nothing, and replacing real-if-coarse provenance with a fabricated precise one is strictly worse: +/// coarse provenance is recoverable, wrong provenance is indistinguishable from right. +/// +/// +internal static class SourceTurnProvenance +{ + /// + /// The message ids to attribute an item to: the single named turn, or + /// unchanged when no usable turn was reported. + /// + internal static IReadOnlyList Resolve( + int? sourceTurn, IReadOnlyList batchMessageIds) + { + if (sourceTurn is not { } turn) return batchMessageIds; + if (turn < 1 || turn > batchMessageIds.Count) return batchMessageIds; + return [batchMessageIds[turn - 1]]; + } + + /// + /// Whether actually narrowed the attribution, for telemetry and for + /// the falsifier — a resolver that silently never fires looks identical to one that always does. + /// + internal static bool Narrowed(int? sourceTurn, IReadOnlyList batchMessageIds) => + sourceTurn is { } turn && turn >= 1 && turn <= batchMessageIds.Count + && batchMessageIds.Count > 1; +} diff --git a/src/AgentMemory.Core/Extraction/WriteTimeFactResolution.cs b/src/AgentMemory.Core/Extraction/WriteTimeFactResolution.cs new file mode 100644 index 00000000..df6c3d6b --- /dev/null +++ b/src/AgentMemory.Core/Extraction/WriteTimeFactResolution.cs @@ -0,0 +1,56 @@ +using AgentMemory.Abstractions.Domain; +using AgentMemory.Core.Memory; + +namespace AgentMemory.Core.Extraction; + +/// +/// Decides whether a newly written fact replaces what was already stored about its subject, or +/// joins it. +/// +/// +/// +/// The write path appends. A conversation saying "I live in Zurich" three months after "I live in +/// Basel" leaves both live, both retrievable and both equally confident — the graph grows with the +/// conversation rather than with what is true, and recall is then asked to choose between two +/// assertions with nothing to choose on. +/// +/// +/// What this is not. The field's usual approach is a model call classifying each candidate +/// write as ADD/UPDATE/DELETE/NOOP. Three of those four are decidable without asking anything: the +/// exact triple already MERGEs, so NOOP is free; a new object for a functional relation is UPDATE; and +/// everything else is ADD. Deciding them from the store rather than from a model costs no completion, +/// cannot vary between runs, and can be falsified in a unit test instead of measured against a +/// provider. DELETE is the one that genuinely needs the conversation — "forget I ever lived in Basel" +/// is not derivable from a triple — and it is left out rather than approximated. +/// +/// +/// The whole safety property is cardinality. Superseding on any repeated subject+predicate +/// would drop "likes coffee" the moment "likes tea" arrived: a true fact closed, silently, with the +/// graph still looking correct. Only relations the vocabulary declares functional are eligible, and +/// anything undeclared — every event relation, most state relations, and every predicate the extractor +/// invented — is multi-valued. +/// +/// +/// The loser is closed non-destructively: it keeps its content, gains invalidated_at and a +/// :SUPERSEDED_BY edge, leaves live recall, and stays visible to as-of recall. So the worst +/// case of a wrong decision is a fact that must be recovered, not one that is gone. +/// +/// +internal static class WriteTimeFactResolution +{ + /// + /// Whether can supersede an earlier assertion at all. + /// + /// + /// The which is answered by the store, not here: liveness lives in + /// invalidated_at, which the domain record does not carry, so filtering in memory would + /// re-close already-closed facts and fan a supersession chain into a star. This is the gate that + /// decides whether to ask at all — and it is the cheap half, since the overwhelming majority of + /// extracted predicates are multi-valued and cost no query. + /// + internal static bool CanSupersede(Fact fact) + { + ArgumentNullException.ThrowIfNull(fact); + return MemoryRelationCardinality.IsSingleValued(fact.Predicate); + } +} diff --git a/src/AgentMemory.Core/Memory/MemoryRelationCardinality.cs b/src/AgentMemory.Core/Memory/MemoryRelationCardinality.cs new file mode 100644 index 00000000..3b6f332e --- /dev/null +++ b/src/AgentMemory.Core/Memory/MemoryRelationCardinality.cs @@ -0,0 +1,65 @@ +namespace AgentMemory.Core.Memory; + +/// +/// Which relations hold at most one live value per subject. +/// +/// +/// +/// The question write-time supersession has to answer is "does this new assertion replace the old +/// one, or join it?" — and getting it wrong in the replacing direction is a data-shaped defect: +/// storing "likes tea" would drop "likes coffee" from live recall, with nothing to indicate that a +/// true fact had been closed. +/// +/// +/// So this answers for everything it has not been explicitly told about. +/// Every event relation is additive by nature — a person attends many things, buys many things +/// — and most state relations are too. The functional set is a small, reviewed handful +/// declared in the vocabulary artifact beside each relation, with the argument for it recorded next to +/// it. +/// +/// +/// Matching is on the canonical predicate key, so a fact stored as lived in or +/// lives_in resolves to the same relation the declaration names. An unrecognised predicate — +/// one the extractor invented outside the vocabulary — is multi-valued, which is the safe answer for +/// something nobody has reviewed. +/// +/// +internal static class MemoryRelationCardinality +{ + private const string Single = "single"; + + private static readonly Lazy> SingleValued = new(() => + { + var document = RelationVocabularyDocument.Load(); + return document.Canonical + .Where(entry => string.Equals(entry.Value.Cardinality, Single, StringComparison.OrdinalIgnoreCase)) + .Select(entry => MemoryTripleCanonicalizer.Canonical(entry.Key)) + .Where(key => key.Length > 0) + .ToHashSet(StringComparer.Ordinal); + }); + + /// + /// Whether holds at most one live value per subject. + /// + /// + /// Resolves surface forms through first, so a predicate the + /// extractor wrote as lived in is recognised as the relation the vocabulary declares. False + /// for anything unrecognised. + /// + internal static bool IsSingleValued(string? predicate) + { + if (string.IsNullOrWhiteSpace(predicate)) return false; + var canonical = MemoryTripleCanonicalizer.Canonical(predicate); + if (canonical.Length == 0) return false; + if (SingleValued.Value.Contains(canonical)) return true; + + // A surface form of a functional relation is that relation. Without this, "lived in" would + // accumulate beside "lives in" and the two would both be live, which is the accumulation this + // exists to stop. + var resolved = MemoryRelationLexicon.Default.Resolve(canonical); + return resolved is not null && SingleValued.Value.Contains(resolved); + } + + /// The declared functional relations, for reporting and for the guard test. + internal static IReadOnlyCollection SingleValuedPredicates => SingleValued.Value; +} diff --git a/src/AgentMemory.Core/Memory/RelationVocabularyDocument.cs b/src/AgentMemory.Core/Memory/RelationVocabularyDocument.cs index 334660e1..53c21be4 100644 --- a/src/AgentMemory.Core/Memory/RelationVocabularyDocument.cs +++ b/src/AgentMemory.Core/Memory/RelationVocabularyDocument.cs @@ -37,6 +37,29 @@ internal sealed class RelationVocabularyEntry /// [JsonPropertyName("storedOnly")] public IReadOnlyList StoredOnly { get; init; } = []; + + /// + /// single when a subject can hold only one live value for this relation; anything else + /// (including absence) means many. + /// + /// + /// + /// Read only by write-time supersession, and defaulting to many is the safety property. A + /// subject can like several things, own several things, and attend several events; superseding on + /// a repeated predicate would silently drop "likes coffee" the moment "likes tea" arrived. Every + /// event relation is additive by nature, and most state relations are too, so declaring the + /// functional handful is far safer than declaring the multi-valued majority and getting one wrong. + /// + /// + /// Authored here, beside the relation itself, rather than as a list in code: it is a judgement + /// about each relation's meaning, it will be argued with, and it belongs where a reviewer reads + /// the relation. cardinalityWhy carries the argument. Not part of the vocabulary + /// fingerprint — that hashes canonical names against surface forms, which decide what is stored + /// and retrieved — so annotating it leaves every sealed base comparable. + /// + /// + [JsonPropertyName("cardinality")] + public string? Cardinality { get; init; } } internal sealed class RelationVocabularyDocument diff --git a/src/AgentMemory.Core/Memory/relation-vocabulary.json b/src/AgentMemory.Core/Memory/relation-vocabulary.json index 62adb79c..5f444fdd 100644 --- a/src/AgentMemory.Core/Memory/relation-vocabulary.json +++ b/src/AgentMemory.Core/Memory/relation-vocabulary.json @@ -193,7 +193,9 @@ "joins", "member of", "played for the" - ] + ], + "cardinality": "single", + "cardinalityWhy": "One owner at a time; a transfer is a replacement, not an addition." }, "booked": { "family": "event", @@ -376,7 +378,9 @@ "cost", "cost me", "priced at" - ] + ], + "cardinality": "single", + "cardinalityWhy": "The current price of one thing. A new price replaces the old." }, "created": { "family": "event", @@ -540,7 +544,9 @@ "expired", "expiring", "expiry" - ] + ], + "cardinality": "single", + "cardinalityWhy": "One expiry instant per subject. Two live expiry dates for the same thing is a contradiction, not two facts." }, "feels": { "family": "state", @@ -866,7 +872,9 @@ "resides in", "residing in", "stays in" - ] + ], + "cardinality": "single", + "cardinalityWhy": "A person has one current residence. 'moved to' is the event that changes it." }, "lost": { "family": "event", @@ -1584,7 +1592,9 @@ "surfaceForms": [ "weigh", "weight" - ] + ], + "cardinality": "single", + "cardinalityWhy": "A measurement of one subject at one time. A new reading replaces the old." }, "welcomed": { "family": "event", @@ -1637,7 +1647,9 @@ "work", "working", "works for" - ] + ], + "cardinality": "single", + "cardinalityWhy": "One current employer. Second jobs exist, but the corpus's 'works at' assertions replace rather than accumulate, and a second employer is expressible as a separate subject-qualified fact." }, "works on": { "family": "state", diff --git a/src/AgentMemory.Extraction.Llm/ExtractionPromptSemantics.cs b/src/AgentMemory.Extraction.Llm/ExtractionPromptSemantics.cs index 0587e8c2..977f498c 100644 --- a/src/AgentMemory.Extraction.Llm/ExtractionPromptSemantics.cs +++ b/src/AgentMemory.Extraction.Llm/ExtractionPromptSemantics.cs @@ -31,6 +31,30 @@ internal static class ExtractionPromptSemantics /// variable here — they are fingerprinted into every run and feed the frozen batch plan's token /// accounting — so a default that shifted them would invalidate sealed bases silently. /// + /// + /// Asks for the source turn's role on every fact and preference. Appended to — and only to — the + /// non- instructions. + /// + /// + /// + /// Trust is stamped once per extraction request, so the moment assistant content is extracted a + /// single batch contains both a user's claims and the model's own, recorded identically. This is + /// the only signal that separates them without a second extraction call, and cost per ingested + /// conversation is a headline number here — a role-split extraction would double it. + /// + /// + /// It is attached here, rather than added to the base prompt, so that the + /// prompt stays byte-for-byte what it was. That is not + /// tidiness: prompt bytes are fingerprinted into every measured run, and at Ignore nothing + /// assistant-derived is extracted at all, so the field would have exactly one possible value and + /// would buy nothing for the base it invalidated. + /// + /// + internal const string SourceRoleInstruction = + "\nOn every fact and preference, add \"source_role\":\"user\" or \"source_role\":\"assistant\" " + + "to record which turn it came from. Use \"assistant\" only when the assistant's own turn is " + + "what states it; if the user said it, or if you are unsure, use \"user\"."; + internal static string AssistantContentInstruction(AssistantContentMode mode) => mode switch { AssistantContentMode.Ignore => string.Empty, @@ -44,14 +68,16 @@ internal static class ExtractionPromptSemantics "subject: for example {\"subject\":\"assistant\",\"predicate\":\"recommended\"," + "\"object\":\"\"}. Prefer recommended, told, provided, suggested, " + "explained. Record only that the assistant said it — do not treat the content as true, " + - "and do not assert it as a fact about the world.", + "and do not assert it as a fact about the world." + + SourceRoleInstruction, // Records the claim itself as a world fact. Stronger subject-matter recall, and a real // hazard, which is why it is opt-in and separately named rather than folded into Utterance. AssistantContentMode.Fact => "\nAlso extract the information the assistant provides as ordinary facts about their " + "subjects, not about the user: from a recommendation of a film, extract facts about that " + - "film. Use the assistant's statements as the source of these facts.", + "film. Use the assistant's statements as the source of these facts." + + SourceRoleInstruction, _ => string.Empty, }; @@ -71,6 +97,33 @@ internal static class ExtractionPromptSemantics /// do otherwise, rather than leaving the model to infer that omission is allowed. /// /// + /// + /// The instruction asking which numbered turn stated each item, or empty for + /// . + /// + /// + /// + /// Only meaningful alongside a numbered transcript — the two ship together, because an instruction + /// naming turn numbers against an unnumbered transcript asks for something the model can only + /// invent. + /// + /// + /// "Omit when unsure" is the load-bearing clause, for the same reason it is on temporal + /// validity. A resolved turn replaces the batch links, so a guessed number does not merely + /// add noise — it discards the true source and substitutes a wrong one, and the result is + /// indistinguishable afterwards from precise attribution. + /// + /// + internal static string ProvenanceInstruction(ExtractionProvenanceMode mode) => mode switch + { + ExtractionProvenanceMode.PerItem => + "\nEach turn in the conversation is numbered as [N]. On every fact and preference, add " + + "\"source_turn\":N naming the single turn that states it. Use the turn where the " + + "information is actually given, not one that merely refers to it, and omit the field " + + "entirely when no single turn states it or you are unsure - never guess a number.", + _ => string.Empty, + }; + internal static string TemporalValidityInstruction(TemporalValidityMode mode) => mode switch { TemporalValidityMode.Extract => diff --git a/src/AgentMemory.Extraction.Llm/Internal/LlmResponseModels.cs b/src/AgentMemory.Extraction.Llm/Internal/LlmResponseModels.cs index 9837df5f..56d32572 100644 --- a/src/AgentMemory.Extraction.Llm/Internal/LlmResponseModels.cs +++ b/src/AgentMemory.Extraction.Llm/Internal/LlmResponseModels.cs @@ -51,6 +51,15 @@ internal sealed class LlmFactDto [JsonPropertyName("valid_until")] public DateTimeOffset? ValidUntil { get; set; } + + // Which turn stated this. Null unless AssistantContentMode asked for it, and null is meaningful: + // it leaves the request's own trust level applying, exactly as before this field existed. + [JsonPropertyName("source_role")] + public string? SourceRole { get; set; } + + // Which numbered turn stated it. Null unless ExtractionProvenanceMode.PerItem asked for it. + [JsonPropertyName("source_turn")] + public int? SourceTurn { get; set; } } internal sealed class LlmPreferenceDto @@ -69,6 +78,14 @@ internal sealed class LlmPreferenceDto [JsonPropertyName("confidence")] public double Confidence { get; set; } = 0.85; + + /// + [JsonPropertyName("source_role")] + public string? SourceRole { get; set; } + + /// + [JsonPropertyName("source_turn")] + public int? SourceTurn { get; set; } } internal sealed class LlmRelationshipDto diff --git a/src/AgentMemory.Extraction.Llm/LlmExtractionOptions.cs b/src/AgentMemory.Extraction.Llm/LlmExtractionOptions.cs index 8486945d..2707dc5b 100644 --- a/src/AgentMemory.Extraction.Llm/LlmExtractionOptions.cs +++ b/src/AgentMemory.Extraction.Llm/LlmExtractionOptions.cs @@ -68,6 +68,18 @@ public sealed class LlmExtractionOptions public TemporalValidityMode TemporalValidity { get; set; } = TemporalValidityMode.Ignore; + /// + /// How precisely a stored fact or preference is bound to the turn that stated it. + /// + /// + /// Defaults to , which appends nothing to any prompt + /// and leaves the transcript unnumbered, so every prompt is byte-for-byte what existing + /// measurements were taken with. changes the + /// rendered conversation as well as the instruction, so it is a stated per-run decision rather + /// than something a package upgrade can turn on. + /// + public ExtractionProvenanceMode Provenance { get; set; } = ExtractionProvenanceMode.Batch; + public bool UseUnifiedExtraction { get; set; } /// diff --git a/src/AgentMemory.Extraction.Llm/LlmFactExtractor.cs b/src/AgentMemory.Extraction.Llm/LlmFactExtractor.cs index 5bff5fcf..7376919d 100644 --- a/src/AgentMemory.Extraction.Llm/LlmFactExtractor.cs +++ b/src/AgentMemory.Extraction.Llm/LlmFactExtractor.cs @@ -49,10 +49,13 @@ public LlmFactExtractor( protected override async Task> ExtractCoreAsync( IReadOnlyList messages, CancellationToken cancellationToken) { - var conversationText = ConversationTextBuilder.Build(messages); + var conversationText = _options.Provenance == ExtractionProvenanceMode.PerItem + ? ConversationTextBuilder.BuildNumbered(messages) + : ConversationTextBuilder.Build(messages); return await _runner.RunAsync( _options.FactExtractionPrompt - ?? BuildSystemPrompt(_options.AssistantContent, _options.TemporalValidity), + ?? BuildSystemPrompt( + _options.AssistantContent, _options.TemporalValidity, _options.Provenance), "Extract facts from this conversation:", conversationText, ProjectFacts, @@ -74,7 +77,9 @@ private static IReadOnlyList ProjectFacts(LlmExtractionResponse d Object = f.Object, Confidence = f.Confidence, ValidFrom = f.ValidFrom, - ValidUntil = f.ValidUntil + ValidUntil = f.ValidUntil, + SourceRole = f.SourceRole, + SourceTurn = f.SourceTurn }) .ToList(); } @@ -90,7 +95,15 @@ internal static string BuildSystemPrompt(AssistantContentMode assistantContent) /// internal static string BuildSystemPrompt( AssistantContentMode assistantContent, TemporalValidityMode temporalValidity) => + BuildSystemPrompt(assistantContent, temporalValidity, ExtractionProvenanceMode.Batch); + + /// + internal static string BuildSystemPrompt( + AssistantContentMode assistantContent, + TemporalValidityMode temporalValidity, + ExtractionProvenanceMode provenance) => DefaultSystemPrompt + ExtractionPromptSemantics.AssistantContentInstruction(assistantContent) - + ExtractionPromptSemantics.TemporalValidityInstruction(temporalValidity); + + ExtractionPromptSemantics.TemporalValidityInstruction(temporalValidity) + + ExtractionPromptSemantics.ProvenanceInstruction(provenance); } diff --git a/src/AgentMemory.Extraction.Llm/LlmMultiSessionUnifiedMemoryExtractor.cs b/src/AgentMemory.Extraction.Llm/LlmMultiSessionUnifiedMemoryExtractor.cs index 72f7b570..c2036889 100644 --- a/src/AgentMemory.Extraction.Llm/LlmMultiSessionUnifiedMemoryExtractor.cs +++ b/src/AgentMemory.Extraction.Llm/LlmMultiSessionUnifiedMemoryExtractor.cs @@ -1,3 +1,4 @@ +using System.Globalization; using System.Text; using AgentMemory.Abstractions.Diagnostics; using AgentMemory.Abstractions.Domain; @@ -50,13 +51,15 @@ Use empty arrays when a category has no supported memory. Do not emit prose or m internal static string BuildSystemPrompt( MemoryPredicateVocabulary? vocabulary, AssistantContentMode assistantContent = AssistantContentMode.Ignore, - TemporalValidityMode temporalValidity = TemporalValidityMode.Ignore) + TemporalValidityMode temporalValidity = TemporalValidityMode.Ignore, + ExtractionProvenanceMode provenance = ExtractionProvenanceMode.Batch) { - // Both shared instructions, appended in the same order every rung uses. A setting honoured by + // Every shared instruction, appended in the same order every rung uses. A setting honoured by // only some extractors is worse than no setting - it makes behaviour depend on a performance // flag - and this rung was the one my first pass missed. var assistant = ExtractionPromptSemantics.AssistantContentInstruction(assistantContent) - + ExtractionPromptSemantics.TemporalValidityInstruction(temporalValidity); + + ExtractionPromptSemantics.TemporalValidityInstruction(temporalValidity) + + ExtractionPromptSemantics.ProvenanceInstruction(provenance); var established = vocabulary?.Snapshot() ?? []; if (established.Count == 0) return SystemPrompt + assistant; @@ -273,9 +276,10 @@ private async Task> Extract Task>> RunProviderAsync() => runner.RunAsync( BuildSystemPrompt( - ActiveVocabulary, _options.AssistantContent, _options.TemporalValidity), + ActiveVocabulary, _options.AssistantContent, _options.TemporalValidity, + _options.Provenance), UserInstruction, - BuildBatchText(batch), + BuildBatchText(batch, _options.Provenance), response => new[] { ProjectAndValidate(response, batch) }, cancellationToken, failOnParseExhaustion: true, @@ -332,6 +336,15 @@ private static IReadOnlyDictionary ProjectAndVa Predicate = item.Predicate, Object = item.Object, Confidence = item.Confidence, + // This rung asks for valid_from/valid_until whenever TemporalValidityMode.Extract is + // set -- the instruction is shared -- but used to drop both on the floor here, so the + // setting was silently a no-op under multi-session extraction. That is precisely the + // "a setting only some extractors respect" defect ExtractionPromptSemantics exists to + // prevent, arriving through the projection instead of the prompt. + ValidFrom = item.ValidFrom, + ValidUntil = item.ValidUntil, + SourceRole = item.SourceRole, + SourceTurn = item.SourceTurn, }); } foreach (var item in response.Preferences ?? []) @@ -344,6 +357,8 @@ private static IReadOnlyDictionary ProjectAndVa PreferenceText = item.Preference, Context = item.Context, Confidence = item.Confidence, + SourceRole = item.SourceRole, + SourceTurn = item.SourceTurn, }); } foreach (var item in response.Relations ?? []) @@ -409,21 +424,33 @@ private int EstimateInputTokens(IReadOnlyList batch) => // Token accounting must see the SAME prompt the call will send, or the frozen batch plan // under-estimates by exactly the instruction it forgot. Encoding.UTF8.GetByteCount(BuildSystemPrompt( - ActiveVocabulary, _options.AssistantContent, _options.TemporalValidity)) + + ActiveVocabulary, _options.AssistantContent, _options.TemporalValidity, + _options.Provenance)) + Encoding.UTF8.GetByteCount(UserInstruction) + - Encoding.UTF8.GetByteCount(BuildBatchText(batch)) + + Encoding.UTF8.GetByteCount(BuildBatchText(batch, _options.Provenance)) + 35); - private static string BuildBatchText(IReadOnlyList batch) + private static string BuildBatchText( + IReadOnlyList batch, + ExtractionProvenanceMode provenance = ExtractionProvenanceMode.Batch) { + var numbered = provenance == ExtractionProvenanceMode.PerItem; var builder = new StringBuilder(); for (var index = 0; index < batch.Count; index++) { var request = batch[index]; builder.Append(""); - foreach (var message in request.Messages) + for (var turn = 0; turn < request.Messages.Count; turn++) { + var message = request.Messages[turn]; + // Numbered WITHIN each source session, restarting at 1. A batch-global number would be + // unresolvable: results are demultiplexed back per session, and each session's own + // source-message ids are what a turn has to index into. + if (numbered) + builder.Append('[') + .Append((turn + 1).ToString(CultureInfo.InvariantCulture)) + .Append("] "); builder.Append('[').Append(message.TimestampUtc.ToString("O")).Append("] ") .Append(message.Role).Append(": ").AppendLine(message.Content); } diff --git a/src/AgentMemory.Extraction.Llm/LlmPreferenceExtractor.cs b/src/AgentMemory.Extraction.Llm/LlmPreferenceExtractor.cs index 4f8b8cba..bdabaa4f 100644 --- a/src/AgentMemory.Extraction.Llm/LlmPreferenceExtractor.cs +++ b/src/AgentMemory.Extraction.Llm/LlmPreferenceExtractor.cs @@ -2,6 +2,7 @@ using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using AgentMemory.Abstractions.Domain; +using AgentMemory.Abstractions.Options; using AgentMemory.Abstractions.Services; using AgentMemory.Core.Extraction; using AgentMemory.Extraction.Llm.Internal; @@ -47,7 +48,9 @@ public LlmPreferenceExtractor( protected override async Task> ExtractCoreAsync( IReadOnlyList messages, CancellationToken cancellationToken) { - var conversationText = ConversationTextBuilder.Build(messages); + var conversationText = _options.Provenance == ExtractionProvenanceMode.PerItem + ? ConversationTextBuilder.BuildNumbered(messages) + : ConversationTextBuilder.Build(messages); return await _runner.RunAsync( _options.PreferenceExtractionPrompt ?? DefaultSystemPrompt, "Extract preferences from this conversation:", @@ -68,7 +71,9 @@ private static IReadOnlyList ProjectPreferences(LlmExtracti Category = p.Category, PreferenceText = p.Preference, Context = string.IsNullOrWhiteSpace(p.Context) ? null : p.Context, - Confidence = p.Confidence + Confidence = p.Confidence, + SourceRole = p.SourceRole, + SourceTurn = p.SourceTurn }) .ToList(); } diff --git a/src/AgentMemory.Extraction.Llm/LlmUnifiedMemoryExtractor.cs b/src/AgentMemory.Extraction.Llm/LlmUnifiedMemoryExtractor.cs index 06659d54..e101d4fa 100644 --- a/src/AgentMemory.Extraction.Llm/LlmUnifiedMemoryExtractor.cs +++ b/src/AgentMemory.Extraction.Llm/LlmUnifiedMemoryExtractor.cs @@ -54,9 +54,12 @@ public async Task ExtractAsync( using var activity = AgentMemoryDiagnostics.Source.StartActivity("memory.extract.unified"); var results = await _runner.RunAsync( BuildSystemPrompt( - _options.AssistantContent, _options.EntityTypes, _options.TemporalValidity), + _options.AssistantContent, _options.EntityTypes, _options.TemporalValidity, + _options.Provenance), "Extract all supported memory from this conversation:", - ConversationTextBuilder.Build(messages), + _options.Provenance == ExtractionProvenanceMode.PerItem + ? ConversationTextBuilder.BuildNumbered(messages) + : ConversationTextBuilder.Build(messages), response => new[] { Project(response) }, cancellationToken, failOnParseExhaustion: true).ConfigureAwait(false); @@ -89,6 +92,8 @@ private static UnifiedExtractionResult Project(LlmExtractionResponse response) = Confidence = item.Confidence, ValidFrom = item.ValidFrom, ValidUntil = item.ValidUntil, + SourceRole = item.SourceRole, + SourceTurn = item.SourceTurn, }).ToArray(), Preferences = (response.Preferences ?? []) .Where(item => !string.IsNullOrWhiteSpace(item.Preference)) @@ -98,6 +103,8 @@ private static UnifiedExtractionResult Project(LlmExtractionResponse response) = PreferenceText = item.Preference, Context = item.Context, Confidence = item.Confidence, + SourceRole = item.SourceRole, + SourceTurn = item.SourceTurn, }).ToArray(), Relationships = (response.Relations ?? []) .Where(item => !string.IsNullOrWhiteSpace(item.Source) && @@ -149,13 +156,23 @@ internal static string BuildSystemPrompt( internal static string BuildSystemPrompt( AssistantContentMode assistantContent, IReadOnlyList entityTypes, - TemporalValidityMode temporalValidity) + TemporalValidityMode temporalValidity) => + BuildSystemPrompt( + assistantContent, entityTypes, temporalValidity, ExtractionProvenanceMode.Batch); + + /// + internal static string BuildSystemPrompt( + AssistantContentMode assistantContent, + IReadOnlyList entityTypes, + TemporalValidityMode temporalValidity, + ExtractionProvenanceMode provenance) { var types = entityTypes is { Count: > 0 } ? entityTypes : LlmEntityExtractor.DefaultEntityTypes; return SystemPromptPrefix + string.Join('|', types) + SystemPromptSuffix + ExtractionPromptSemantics.AssistantContentInstruction(assistantContent) - + ExtractionPromptSemantics.TemporalValidityInstruction(temporalValidity); + + ExtractionPromptSemantics.TemporalValidityInstruction(temporalValidity) + + ExtractionPromptSemantics.ProvenanceInstruction(provenance); } } diff --git a/src/AgentMemory.McpServer/AgentMemoryMcpOptions.cs b/src/AgentMemory.McpServer/AgentMemoryMcpOptions.cs index 57d7b001..bba37fec 100644 --- a/src/AgentMemory.McpServer/AgentMemoryMcpOptions.cs +++ b/src/AgentMemory.McpServer/AgentMemoryMcpOptions.cs @@ -30,4 +30,21 @@ public sealed class AgentMemoryMcpOptions /// Default confidence score for entities, facts, and preferences added via MCP tools. /// public double DefaultConfidence { get; set; } = 0.9; + + /// + /// Exposes only the tools that read. Every tool that creates, modifies, invalidates or derives + /// stored memory is removed from the server's tool list entirely. + /// + /// + /// + /// Removed, not refused at call time: a tool the client can see is a tool a model will try, and an + /// error return teaches it nothing about what the server is for. A read-only server should look + /// like a read-only server in the handshake. + /// + /// + /// The classification is , where writes are the enumerated set, + /// so a tool added later and left unclassified is withheld rather than silently exposed. + /// + /// + public bool ReadOnly { get; set; } } diff --git a/src/AgentMemory.McpServer/McpToolAccess.cs b/src/AgentMemory.McpServer/McpToolAccess.cs new file mode 100644 index 00000000..1a0e3ef3 --- /dev/null +++ b/src/AgentMemory.McpServer/McpToolAccess.cs @@ -0,0 +1,77 @@ +namespace AgentMemory.McpServer; + +/// +/// Which of the memory tools change stored state. +/// +/// +/// +/// This exists so --read-only can be a fact rather than a promise. A read-only server that +/// still exposes a write tool is worse than one with no such mode, because the operator believes the +/// guarantee and stops checking. +/// +/// +/// Writes are the enumerated set, deliberately. Listing the reads instead would mean a tool +/// added later and forgotten defaults to "not a write" and stays exposed in read-only mode — the +/// failure lands silently on the safety side. Enumerating writes makes the same forgetfulness fail +/// the other way: the new tool is treated as a write, disappears from read-only servers, and somebody +/// notices. A guard test asserts every registered tool is named in exactly one of the two lists, so +/// the choice is enforced rather than hoped for. +/// +/// +/// graph_query is a write for this purpose even though it is nominally a read: it takes +/// arbitrary Cypher. It is already gated behind EnableGraphQuery and validated read-only at +/// execution, but a mode whose entire value is "this cannot change anything" should not rest on a +/// second component's parser. +/// +/// +public static class McpToolAccess +{ + /// Tools that create, modify, invalidate or derive stored memory. + public static IReadOnlySet WriteTools { get; } = new HashSet(StringComparer.Ordinal) + { + "memory_store_message", + "memory_add_entity", + "memory_add_preference", + "memory_add_fact", + "memory_create_relationship", + "memory_record_entity_feedback", + "memory_start_trace", + "memory_record_step", + "memory_complete_trace", + "memory_record_tool_call", + "extract_and_persist", + "memory_extract_session", + "memory_generate_embeddings", + "memory_invalidate", + "memory_supersede", + // Arbitrary Cypher. Read-only in intent and validated as such at execution, but a mode that + // exists to guarantee "nothing changes" must not delegate that guarantee to a query parser. + "graph_query", + }; + + /// Tools that only read. + /// + /// Named explicitly rather than derived as "everything else", so that the guard test can prove no + /// tool falls outside both sets — which is the only way "read-only" stays true as tools are added. + /// + public static IReadOnlySet ReadTools { get; } = new HashSet(StringComparer.Ordinal) + { + "memory_search", + "memory_get_context", + "memory_get_conversation", + "memory_list_sessions", + "memory_get_entity", + "memory_get_entity_provenance", + "memory_get_observations", + "memory_export_graph", + "memory_find_duplicates", + }; + + /// Whether may run on a read-only server. + /// + /// An unrecognised name is treated as a write. A tool this class has never heard of is a tool + /// nobody has classified, and the safe reading of "unclassified" is "assume it changes something". + /// + public static bool IsReadOnly(string? toolName) => + toolName is not null && ReadTools.Contains(toolName); +} diff --git a/src/AgentMemory.McpServer/ServiceCollectionExtensions.cs b/src/AgentMemory.McpServer/ServiceCollectionExtensions.cs index acfd5c79..d9006c76 100644 --- a/src/AgentMemory.McpServer/ServiceCollectionExtensions.cs +++ b/src/AgentMemory.McpServer/ServiceCollectionExtensions.cs @@ -30,7 +30,7 @@ public static IMcpServerBuilder AddAgentMemoryMcpTools(this IMcpServerBuilder bu sdk.ServerInfo = new Implementation { Name = o.ServerName, Version = o.ServerVersion }; }); - return builder + builder .WithTools() .WithTools() .WithTools() @@ -39,6 +39,103 @@ public static IMcpServerBuilder AddAgentMemoryMcpTools(this IMcpServerBuilder bu .WithTools() .WithTools() .WithTools(); + + ApplyReadOnlyFilter(builder.Services); + return builder; + } + + /// + /// Removes every write tool from the registration when + /// is set. + /// + /// + /// + /// Applied to the DI descriptors rather than enforced inside each tool, so a read-only server does + /// not advertise what it will refuse. A visible tool is one a model will call, and a runtime + /// error teaches it nothing about the server's purpose. + /// + /// + /// Filtering must happen while the collection is still mutable — after the host is built the tool + /// list is fixed — so the setting is read here rather than at start. + /// + /// + private static void ApplyReadOnlyFilter(IServiceCollection services) + { + if (!ReadOnlyRequested(services)) return; + + // The SDK registers each tool as a singleton FACTORY, not an instance, so the name can only be + // read by invoking it. It builds the McpServerTool from the method's attributes and resolves + // the declaring type lazily at call time, so an empty provider is enough here and nothing that + // touches a database or a model is constructed. + using var empty = new ServiceCollection().BuildServiceProvider(); + + var withheld = services + .Where(descriptor => + descriptor.ServiceType == typeof(McpServerTool) + && !McpToolAccess.IsReadOnly(ToolNameOf(descriptor, empty))) + .ToList(); + + foreach (var descriptor in withheld) services.Remove(descriptor); + } + + /// + /// Whether read-only was configured, without running the options validators. + /// + /// + /// Applies the registered configure callbacks to a fresh instance rather than resolving + /// IOptions, deliberately. Resolving would run ValidateOnStart's validators here, + /// during registration, so an out-of-range DefaultConfidence would surface from + /// AddAgentMemoryMcpTools instead of from host startup — moving where an unrelated + /// misconfiguration is reported, and coupling tool filtering to the validity of a setting it does + /// not use. + /// + private static bool ReadOnlyRequested(IServiceCollection services) + { + var options = new AgentMemoryMcpOptions(); + using var empty = new ServiceCollection().BuildServiceProvider(); + + foreach (var descriptor in services.Where(d => + d.ServiceType == typeof(IConfigureOptions))) + { + try + { + var configure = descriptor.ImplementationInstance as IConfigureOptions + ?? descriptor.ImplementationFactory?.Invoke(empty) as IConfigureOptions; + configure?.Configure(options); + } + catch (Exception) + { + // A configuration source this method cannot evaluate here -- one bound to services that + // only exist in the real provider, say -- must not break registration for every host. + // Skipping it means read-only might not be seen from that source; the flag on the tool + // and the environment variable both reach this instance directly, and the alternative + // is a host that cannot start at all. + } + } + return options.ReadOnly; + } + + /// + /// The advertised name of an already-registered tool descriptor. + /// + /// + /// A descriptor whose name cannot be read returns null, which + /// treats as a write — an unreadable tool is an + /// unclassified one, and unclassified must fail closed. That matters here because the name comes + /// from invoking a factory: if a future SDK version needed real services to build a tool, this + /// would start throwing, and the correct response is to withhold the tool rather than to expose it. + /// + private static string? ToolNameOf(ServiceDescriptor descriptor, IServiceProvider provider) + { + try + { + return (descriptor.ImplementationInstance as McpServerTool)?.ProtocolTool.Name + ?? (descriptor.ImplementationFactory?.Invoke(provider) as McpServerTool)?.ProtocolTool.Name; + } + catch (Exception) + { + return null; + } } /// diff --git a/src/AgentMemory.Neo4j/Queries/FactQueries.cs b/src/AgentMemory.Neo4j/Queries/FactQueries.cs index 95696f24..a9f9c7b9 100644 --- a/src/AgentMemory.Neo4j/Queries/FactQueries.cs +++ b/src/AgentMemory.Neo4j/Queries/FactQueries.cs @@ -176,6 +176,45 @@ public static string GetBySubject(bool hasOwnerFilter, bool includeShared) return $"MATCH (f:Fact) WHERE f.subject = $subject{owner} RETURN f"; } + // ── Write-time supersession (M1) ─────────────────────────────────── + + /// + /// The live facts that assert a different object for the same subject and predicate, which a + /// newly written fact about a functional relation replaces. + /// + /// + /// + /// Liveness is filtered here rather than in memory for the reason every other liveness filter is: + /// invalidated_at is not carried on the domain record, so a caller holding a + /// Fact cannot tell a closed one from a live one. Fetching every fact for a subject and + /// filtering afterwards would re-supersede already-closed facts — harmless to their timestamps, + /// which coalesce protects, but it would accumulate a second :SUPERSEDED_BY edge per + /// arrival and turn a chain into a fan. + /// + /// + /// Excludes the winner by id: a fact cannot supersede itself, and the MERGE-on-triple write path + /// means the incoming fact is already stored when this runs. + /// + /// + /// (A method, not a const, so it is excluded from the Cypher snapshot inventory.) + /// + /// + public static string FindSupersededCandidates(bool hasOwnerFilter, bool includeShared) + { + var owner = !hasOwnerFilter ? string.Empty + : includeShared ? " AND (f.owner_id = $ownerId OR f.owner_id IS NULL)" + : " AND f.owner_id = $ownerId"; + return @" + MATCH (f:Fact) + WHERE f.subject_key = $subjectKey + AND f.predicate_key = $predicateKey + AND f.object_key <> $objectKey + AND f.id <> $winnerId + AND f.invalidated_at IS NULL" + owner + @" + RETURN f + ORDER BY f.created_at DESC"; + } + // ── Dedup-on-create ──────────────────────────────────────────────── /// diff --git a/src/AgentMemory.Neo4j/Queries/ReasoningQueries.cs b/src/AgentMemory.Neo4j/Queries/ReasoningQueries.cs index d6d36f64..182674af 100644 --- a/src/AgentMemory.Neo4j/Queries/ReasoningQueries.cs +++ b/src/AgentMemory.Neo4j/Queries/ReasoningQueries.cs @@ -1,4 +1,4 @@ -namespace AgentMemory.Neo4j.Queries; +namespace AgentMemory.Neo4j.Queries; /// /// Centralized Cypher queries for ReasoningTrace and ReasoningStep operations. @@ -16,7 +16,8 @@ internal static class ReasoningQueries task: $task, outcome: $outcome, success: $success, - metadata: $metadata + metadata: $metadata, + trace_kind: $traceKind }) SET t.started_at = datetime($startedAt), t.completed_at = CASE WHEN $completedAt IS NOT NULL THEN datetime($completedAt) ELSE null END @@ -114,9 +115,16 @@ public static string PruneSessionTraces(bool ownerIsShared) // A retention prune is a DESTRUCTIVE write keyed by a guessable session_id, so its owner clause must // NEVER collapse to "all owners" (the #40 regression). It always confines to a single bucket. var owner = ownerIsShared ? " AND t.owner_id IS NULL" : " AND t.owner_id = $ownerId"; + // PROMOTED PROCEDURES ARE EXEMPT. This prune orders by started_at with age as its ONLY + // criterion and fires on every trace creation once a per-session cap is set -- so without this + // clause a promoted procedure is deleted by recency and the capability does not exist at all. + // Written as "is NOT a procedure" rather than "is an episode" on purpose: a trace stored before + // trace_kind existed has the property NULL, and a NULL-unsafe comparison would exempt every + // legacy trace from retention, quietly turning a bounded store into an unbounded one. + const string exemption = " AND coalesce(t.trace_kind, 'episode') <> 'procedure'"; return @" MATCH (t:ReasoningTrace {session_id: $sessionId}) - WHERE true" + owner + @" + WHERE true" + owner + exemption + @" WITH t ORDER BY t.started_at DESC SKIP $keep OPTIONAL MATCH (t)-[:HAS_STEP]->(s:ReasoningStep) @@ -135,10 +143,20 @@ WITH collect(DISTINCT t) AS traces, collect(DISTINCT s) AS steps, count(DISTINCT /// reached the querying owner on a 50-owner corpus). Unlike the fact path, trace search does /// not escalate on an empty scoped result. /// - public static string SearchByTaskVector(bool hasSuccessFilter, bool hasOwnerFilter, bool includeShared, int topK) + public static string SearchByTaskVector( + bool hasSuccessFilter, bool hasOwnerFilter, bool includeShared, int topK, + bool? proceduresOnly = null) { var conditions = new List { "score >= $minScore" }; if (hasSuccessFilter) conditions.Add("node.success = $successFilter"); + // Opt-in and DEFAULT NULL. The TCK bridge's /get_similar_traces takes every default, so a + // non-null default here would change the Cypher it emits and break Gold 18/18 immediately -- + // and it would do so by filtering a corpus that has no promoted traces at all, i.e. to zero. + // Null-safe for the same reason as the prune: a pre-trace_kind trace has the property NULL. + if (proceduresOnly is { } procedures) + conditions.Add(procedures + ? "coalesce(node.trace_kind, 'episode') = 'procedure'" + : "coalesce(node.trace_kind, 'episode') <> 'procedure'"); if (hasOwnerFilter) conditions.Add(includeShared ? "(node.owner_id = $ownerId OR node.owner_id IS NULL)" @@ -257,8 +275,16 @@ RETURN e.id AS id /// have both returned nothing, and bounded by ONE owner's traces rather than by the corpus. The /// success filter is preserved: a filtered search that legitimately matches nothing must still /// return nothing here rather than being rescued into the wrong answer. + /// + /// is preserved for exactly the same reason, and it is the case + /// that actually bites: a corpus with no promoted procedures makes the indexed pass return zero + /// by construction, which is precisely the condition that triggers this rescue. Dropping the + /// filter here would answer "find me a procedure for this task" with an ordinary episode — the one + /// wrong answer procedural recall must never give, and one no caller could detect. + /// /// - public static string SearchByTaskVectorOwnerScopedFallback(bool hasSuccessFilter, bool includeShared) + public static string SearchByTaskVectorOwnerScopedFallback( + bool hasSuccessFilter, bool includeShared, bool? proceduresOnly = null) { var owner = includeShared ? "(t.owner_id = $ownerId OR t.owner_id IS NULL)" @@ -266,10 +292,17 @@ public static string SearchByTaskVectorOwnerScopedFallback(bool hasSuccessFilter var success = hasSuccessFilter ? Environment.NewLine + " AND t.success = $successFilter" : string.Empty; + // Null-safe, as on the indexed path: a trace stored before trace_kind existed has the property + // missing, and a NULL-unsafe comparison would drop the whole pre-migration corpus from the + // episode side of this filter. + var kind = proceduresOnly is { } procedures + ? Environment.NewLine + " AND coalesce(t.trace_kind, 'episode') " + + (procedures ? "=" : "<>") + " 'procedure'" + : string.Empty; return $@" MATCH (t:ReasoningTrace) WHERE {owner} - AND t.task_embedding IS NOT NULL{success} + AND t.task_embedding IS NOT NULL{success}{kind} WITH t, vector.similarity.cosine(t.task_embedding, $embedding) AS score WHERE score >= $minScore RETURN t AS node, score diff --git a/src/AgentMemory.Neo4j/Queries/SchemaQueries.cs b/src/AgentMemory.Neo4j/Queries/SchemaQueries.cs index 0e9c563e..da083dfe 100644 --- a/src/AgentMemory.Neo4j/Queries/SchemaQueries.cs +++ b/src/AgentMemory.Neo4j/Queries/SchemaQueries.cs @@ -184,6 +184,16 @@ internal static class SchemaQueries /// Index on ReasoningTrace.success. public const string TraceSuccessIndex = "CREATE INDEX trace_success_idx IF NOT EXISTS FOR (t:ReasoningTrace) ON (t.success)"; + /// + /// Index on ReasoningTrace.trace_kind — the promotion marker separating an ordinary episode + /// from a reusable procedure. + /// + /// + /// Seekable so a procedures-only search is a seek rather than a post-filter over the whole label, + /// and mirroring , the sibling single-property index here. + /// + public const string TraceKindIndex = "CREATE INDEX trace_kind_idx IF NOT EXISTS FOR (t:ReasoningTrace) ON (t.trace_kind)"; + /// Index on ReasoningStep.timestamp. public const string ReasoningStepTimestampIndex = "CREATE INDEX reasoning_step_timestamp IF NOT EXISTS FOR (s:ReasoningStep) ON (s.timestamp)"; @@ -343,6 +353,7 @@ internal static class SchemaQueries PreferenceCategoryIndex, TraceSessionIndex, TraceSuccessIndex, + TraceKindIndex, ReasoningStepTimestampIndex, ToolCallStatusIndex, SchemaNameIndex, @@ -429,8 +440,9 @@ public static IReadOnlyList BootstrapStatements(int dimensions) /// a failure; it is the normal asynchronous build state. /// public const string ShowIndexStates = - "SHOW INDEXES YIELD name, state, type " + - "RETURN name AS name, state AS state, type AS type"; + "SHOW INDEXES YIELD name, state, type, populationPercent " + + "RETURN name AS name, state AS state, type AS type, " + + "populationPercent AS populationPercent"; // ── Schema-conformance introspection (CLI `schema-check`) ──── diff --git a/src/AgentMemory.Neo4j/Repositories/Neo4jFactRepository.cs b/src/AgentMemory.Neo4j/Repositories/Neo4jFactRepository.cs index a89fe77f..3b34245c 100644 --- a/src/AgentMemory.Neo4j/Repositories/Neo4jFactRepository.cs +++ b/src/AgentMemory.Neo4j/Repositories/Neo4jFactRepository.cs @@ -239,6 +239,37 @@ await runner.RunAsync( }, cancellationToken).ConfigureAwait(false); } + public async Task> FindSupersededCandidatesAsync( + string winnerFactId, + string subject, + string predicate, + string @object, + MemoryScope? scope = null, + CancellationToken cancellationToken = default) + { + bool hasOwner = scope?.HasOwnerFilter == true; + bool includeShared = scope?.IncludeShared ?? true; + + var cypher = FactQueries.FindSupersededCandidates(hasOwner, includeShared); + var parameters = new Dictionary + { + ["winnerId"] = winnerFactId, + ["subjectKey"] = MemoryTripleCanonicalizer.CanonicalValue(subject), + ["predicateKey"] = MemoryTripleCanonicalizer.Canonical(predicate), + ["objectKey"] = MemoryTripleCanonicalizer.CanonicalValue(@object), + }; + if (hasOwner) parameters["ownerId"] = scope!.OwnerId; + + return await _tx.ReadAsync(async runner => + { + var cursor = await runner.RunAsync(cypher, parameters).ConfigureAwait(false); + var records = await cursor.ToListAsync().ConfigureAwait(false); + return (IReadOnlyList)records + .Select(r => MapToFact(r["f"].As(), embedding: null)) + .ToList(); + }, cancellationToken).ConfigureAwait(false); + } + public async Task> GetBySubjectAsync( string subject, MemoryScope? scope = null, CancellationToken cancellationToken = default) { diff --git a/src/AgentMemory.Neo4j/Repositories/Neo4jReasoningTraceRepository.cs b/src/AgentMemory.Neo4j/Repositories/Neo4jReasoningTraceRepository.cs index a69d7894..b58d1013 100644 --- a/src/AgentMemory.Neo4j/Repositories/Neo4jReasoningTraceRepository.cs +++ b/src/AgentMemory.Neo4j/Repositories/Neo4jReasoningTraceRepository.cs @@ -1,4 +1,4 @@ -using System.Diagnostics; +using System.Diagnostics; using Microsoft.Extensions.Logging; using AgentMemory.Abstractions.Diagnostics; using AgentMemory.Abstractions.Domain; @@ -136,8 +136,19 @@ public async Task> ListAllAsync(int limit = 50, int }, cancellationToken).ConfigureAwait(false); } + public Task> SearchByTaskVectorAsync( + float[] taskEmbedding, + bool? successFilter = null, + int limit = 10, + double minScore = 0.0, + MemoryScope? scope = null, + CancellationToken cancellationToken = default) => + SearchByTaskVectorAsync( + taskEmbedding, proceduresOnly: null, successFilter, limit, minScore, scope, cancellationToken); + public async Task> SearchByTaskVectorAsync( float[] taskEmbedding, + bool? proceduresOnly, bool? successFilter = null, int limit = 10, double minScore = 0.0, @@ -164,7 +175,7 @@ public async Task> ListAllAsync(int limit = 50, int async Task> QueryAsync(int width, CancellationToken ct) { var cypher = ReasoningQueries.SearchByTaskVector( - successFilter.HasValue, hasOwner, includeShared, width); + successFilter.HasValue, hasOwner, includeShared, width, proceduresOnly); var parameters = new Dictionary { @@ -215,7 +226,10 @@ public async Task> ListAllAsync(int limit = 50, int // that many more-similar foreign rows no widening reaches the owner's. Bounded by one // owner's traces rather than by the corpus, and reached only when both prior passes // returned nothing. The success filter is carried through, so a filtered search that - // genuinely matches nothing still returns nothing. + // genuinely matches nothing still returns nothing -- and so is proceduresOnly, which is + // the filter most likely to reach this branch: a corpus with no promoted procedures makes + // the indexed pass return zero by construction, so an unfiltered rescue would answer a + // procedure lookup with an episode. if (results.Count == 0) { _logger.LogDebug( @@ -234,7 +248,7 @@ public async Task> ListAllAsync(int limit = 50, int { var cursor = await runner.RunAsync( ReasoningQueries.SearchByTaskVectorOwnerScopedFallback( - successFilter.HasValue, includeShared), + successFilter.HasValue, includeShared, proceduresOnly), fallbackParameters).ConfigureAwait(false); var records = await cursor.ToListAsync().ConfigureAwait(false); return records.Select(r => @@ -457,6 +471,12 @@ private static ReasoningTrace MapToTrace(INode node, float[]? taskEmbedding) => CompletedAtUtc = node.Properties.TryGetValue("completed_at", out var ca) ? Neo4jDateTimeHelper.ReadNullableDateTimeOffset(ca) : null, + // Absent property == Episode. Every trace written before trace_kind existed reads back as + // an episode, which is what it is -- never as an unknown that a caller has to handle. + Kind = node.Properties.TryGetValue("trace_kind", out var tk) + && string.Equals(tk?.As(), "procedure", StringComparison.Ordinal) + ? TraceKind.Procedure + : TraceKind.Episode, Metadata = DeserializeMetadata(node.Properties.TryGetValue("metadata", out var md) ? md.As() : null) }; @@ -476,6 +496,10 @@ private static ReasoningTrace MapToTrace(INode node, float[]? taskEmbedding) => ["success"] = (object?)trace.Success, ["startedAt"] = trace.StartedAtUtc.ToString("O"), ["completedAt"] = (object?)(trace.CompletedAtUtc?.ToString("O")), - ["metadata"] = SerializeMetadata(trace.Metadata) + ["metadata"] = SerializeMetadata(trace.Metadata), + // Written as a lowercase string rather than an int so the stored value is readable in a Cypher + // console and stable if the enum is ever reordered -- an ordinal would silently re-point every + // existing node at a different meaning. + ["traceKind"] = trace.Kind == TraceKind.Procedure ? "procedure" : "episode" }; } \ No newline at end of file diff --git a/src/AgentMemory.Neo4j/Schema/Migrations/0011_trace_kind.cypher b/src/AgentMemory.Neo4j/Schema/Migrations/0011_trace_kind.cypher new file mode 100644 index 00000000..2ff7cd7e --- /dev/null +++ b/src/AgentMemory.Neo4j/Schema/Migrations/0011_trace_kind.cypher @@ -0,0 +1,25 @@ +// Migration 0011 — index ReasoningTrace.trace_kind, the promotion marker procedural memory turns on. +// +// A trace and a procedure are the same record read two ways: an episode says what happened once, a +// procedure says what to do next time. They differ by RETRIEVAL KEY, so the marker has to be seekable +// for the "procedures only" search to be anything other than a post-filter over the whole label. +// +// Named trace_kind and NOT kind, deliberately: `kind` already means "audit-node discriminator" both +// here and upstream, and overloading a property whose meaning is shared with another implementation is +// the changed-semantics hazard the schema-parity check exists to catch. A new PROPERTY is ungated by +// that verifier; a new label or relationship type is not, which is why promotion is a property rather +// than a :Procedure label or a PROMOTED_FROM edge -- strictly more risk, zero more function. +// +// Mirrors trace_success_idx, the sibling single-property index on this label. +// +// It also carries the retention exemption: PruneSessionTraces orders by started_at with age as its +// ONLY criterion and fires on every trace creation once MaxTracesPerSession is set, so a promoted +// procedure without this marker is deleted by recency and the capability does not exist. The prune's +// clause is coalesce(t.trace_kind, 'episode') <> 'procedure' -- NULL-safe on purpose, because a trace +// written before this property existed must still be prunable, or a retention cap silently stops +// capping. +// +// Fresh deployments pick this up via SchemaBootstrapper; this migration brings existing databases to +// parity. Idempotent (IF NOT EXISTS); each statement runs in its own transaction. + +CREATE INDEX trace_kind_idx IF NOT EXISTS FOR (t:ReasoningTrace) ON (t.trace_kind); diff --git a/tests/AgentMemory.Tests.Integration/Repositories/ProcedurePromotionIntegrationTests.cs b/tests/AgentMemory.Tests.Integration/Repositories/ProcedurePromotionIntegrationTests.cs new file mode 100644 index 00000000..96aa8184 --- /dev/null +++ b/tests/AgentMemory.Tests.Integration/Repositories/ProcedurePromotionIntegrationTests.cs @@ -0,0 +1,225 @@ +using FluentAssertions; +using Microsoft.Extensions.Logging.Abstractions; +using AgentMemory.Abstractions.Domain; +using AgentMemory.Abstractions.Options; +using AgentMemory.Neo4j.Repositories; +using AgentMemory.Tests.Integration.Fixtures; + +namespace AgentMemory.Tests.Integration.Repositories; + +/// +/// Procedural memory's two correctness falsifiers, against a live database (PLAN 7.5). +/// +/// +/// +/// Deterministic and model-free. Promotion is not a semantic property, so neither falsifier needs a +/// corpus, an embedding model or a judge — they answer the only two questions that decide whether the +/// capability exists at all: procedure_recall_at_1 (can a procedure be retrieved as one) +/// and promoted_survived_prune (does it survive retention). +/// +/// +/// The second is the load-bearing one. PruneSessionTracesAsync orders by started_at with +/// age as its only criterion, and it fires on every trace creation once +/// MaxTracesPerSession is set. Without the exemption, promoting a trace is undone by the next +/// few turns: the feature would ship, read correctly in every unit test that never prunes, and be a +/// no-op in production. +/// +/// +/// These run against Neo4j rather than a substitute because both properties live entirely in Cypher — +/// the trace_kind predicate on the vector search and the coalesce(...) <> 'procedure' +/// clause in the prune. A mocked repository would assert that the arguments were passed, which is the +/// part that was never in doubt. +/// +/// +[Collection("Neo4j Integration")] +[Trait("Category", "Integration")] +public class ProcedurePromotionIntegrationTests : IAsyncLifetime +{ + private readonly Neo4jIntegrationFixture _fixture; + private readonly Neo4jReasoningTraceRepository _traceRepo; + + private static readonly float[] Embedding = [0.3f, 0.1f, 0.4f, 0.2f]; + + public ProcedurePromotionIntegrationTests(Neo4jIntegrationFixture fixture) + { + _fixture = fixture; + _traceRepo = new Neo4jReasoningTraceRepository( + fixture.TransactionRunner, NullLogger.Instance); + } + + public Task InitializeAsync() => _fixture.CleanDatabaseAsync(); + public Task DisposeAsync() => Task.CompletedTask; + + private static ReasoningTrace MakeTrace( + string sessionId, int dayOffset, TraceKind kind, string? ownerId = "alice") => new() + { + TraceId = $"trace-{Guid.NewGuid():N}", + SessionId = sessionId, + OwnerId = ownerId, + Task = $"Task day {dayOffset}", + TaskEmbedding = Embedding, + Kind = kind, + StartedAtUtc = new DateTimeOffset(2025, 1, 1, 0, 0, 0, TimeSpan.Zero).AddDays(dayOffset) + }; + + // ---- procedure_recall_at_1 ------------------------------------------------ + + [Fact] + public async Task ProceduresOnlySearch_ReturnsThePromotedTrace_AndNotTheEpisode() + { + var sessionId = $"session-{Guid.NewGuid():N}"; + var procedure = MakeTrace(sessionId, 0, TraceKind.Procedure); + var episode = MakeTrace(sessionId, 1, TraceKind.Episode); + await _traceRepo.AddAsync(procedure); + await _traceRepo.AddAsync(episode); + + var hits = await _traceRepo.SearchByTaskVectorAsync( + Embedding, proceduresOnly: true, successFilter: null, limit: 10, minScore: 0.0, + scope: MemoryScope.For("alice")); + + hits.Should().ContainSingle().Which.Trace.TraceId.Should().Be(procedure.TraceId); + hits[0].Trace.Kind.Should().Be(TraceKind.Procedure); + } + + [Fact] + public async Task ExcludingProcedures_ReturnsOnlyEpisodes() + { + // The other direction of the same predicate. A filter that is silently ignored passes the test + // above (the procedure is in the result set either way) and fails only here. + var sessionId = $"session-{Guid.NewGuid():N}"; + var procedure = MakeTrace(sessionId, 0, TraceKind.Procedure); + var episode = MakeTrace(sessionId, 1, TraceKind.Episode); + await _traceRepo.AddAsync(procedure); + await _traceRepo.AddAsync(episode); + + var hits = await _traceRepo.SearchByTaskVectorAsync( + Embedding, proceduresOnly: false, successFilter: null, limit: 10, minScore: 0.0, + scope: MemoryScope.For("alice")); + + hits.Should().ContainSingle().Which.Trace.TraceId.Should().Be(episode.TraceId); + } + + [Fact] + public async Task TheUnfilteredSearch_StillReturnsBothKinds() + { + // The byte-identical guarantee for every existing caller and for third-party implementers of + // IReasoningTraceRepository: no filter means no filtering, not "episodes only". + var sessionId = $"session-{Guid.NewGuid():N}"; + await _traceRepo.AddAsync(MakeTrace(sessionId, 0, TraceKind.Procedure)); + await _traceRepo.AddAsync(MakeTrace(sessionId, 1, TraceKind.Episode)); + + var hits = await _traceRepo.SearchByTaskVectorAsync( + Embedding, successFilter: null, limit: 10, minScore: 0.0, scope: MemoryScope.For("alice")); + + hits.Select(h => h.Trace.Kind).Should().BeEquivalentTo( + new[] { TraceKind.Procedure, TraceKind.Episode }); + } + + [Fact] + public async Task LegacyTracesWithNoStoredKind_AreTreatedAsEpisodes() + { + // Every trace written before 0011 has no trace_kind property at all. The queries coalesce it to + // 'episode'; if that coalesce were dropped, a proceduresOnly:false search would silently stop + // returning the entire pre-migration corpus -- a null-vs-missing bug that no new-data test sees. + var sessionId = $"session-{Guid.NewGuid():N}"; + var legacy = MakeTrace(sessionId, 0, TraceKind.Episode); + await _traceRepo.AddAsync(legacy); + await _fixture.TransactionRunner.WriteAsync(async runner => + { + await runner.RunAsync( + "MATCH (t:ReasoningTrace {id: $id}) REMOVE t.trace_kind", new { id = legacy.TraceId }); + return true; + }); + + var episodes = await _traceRepo.SearchByTaskVectorAsync( + Embedding, proceduresOnly: false, successFilter: null, limit: 10, minScore: 0.0, + scope: MemoryScope.For("alice")); + var procedures = await _traceRepo.SearchByTaskVectorAsync( + Embedding, proceduresOnly: true, successFilter: null, limit: 10, minScore: 0.0, + scope: MemoryScope.For("alice")); + + episodes.Should().ContainSingle().Which.Trace.TraceId.Should().Be(legacy.TraceId, + "a trace stored before the migration is an episode, not an absence"); + procedures.Should().BeEmpty(); + } + + // ---- promoted_survived_prune --------------------------------------------- + + [Fact] + public async Task APromotedProcedure_SurvivesRetentionPruning() + { + // THE falsifier. The procedure is the OLDEST trace in the session, so age alone deletes it. + var sessionId = $"session-{Guid.NewGuid():N}"; + var procedure = MakeTrace(sessionId, 0, TraceKind.Procedure); + await _traceRepo.AddAsync(procedure); + for (var day = 1; day <= 5; day++) + await _traceRepo.AddAsync(MakeTrace(sessionId, day, TraceKind.Episode)); + + var pruned = await _traceRepo.PruneSessionTracesAsync(sessionId, maxToKeep: 2, ownerId: "alice"); + + (await _traceRepo.GetByIdAsync(procedure.TraceId)).Should().NotBeNull( + "a promoted procedure is exempt from recency pruning -- without the exemption, promotion is " + + "undone by the next few traces and the capability does not exist"); + pruned.Should().Be(3, "the 3 oldest EPISODES are evicted; the procedure is not counted or deleted"); + } + + [Fact] + public async Task OrdinaryEpisodes_AreStillPruned() + { + // The exemption must not disable retention. A cap that silently stops capping turns a bounded + // store into an unbounded one, which is worse than having no cap, because nobody looks. + var sessionId = $"session-{Guid.NewGuid():N}"; + var oldest = MakeTrace(sessionId, 0, TraceKind.Episode); + await _traceRepo.AddAsync(oldest); + for (var day = 1; day <= 5; day++) + await _traceRepo.AddAsync(MakeTrace(sessionId, day, TraceKind.Episode)); + + await _traceRepo.PruneSessionTracesAsync(sessionId, maxToKeep: 2, ownerId: "alice"); + + (await _traceRepo.GetByIdAsync(oldest.TraceId)).Should().BeNull(); + } + + [Fact] + public async Task ProceduresDoNotConsumeTheRetentionBudget() + { + // The subtle failure the exemption could still have: exempting a procedure from DELETION while + // still COUNTING it toward maxToKeep would evict a live episode to make room for a trace that + // was never at risk -- retention silently tightening as procedures accumulate. + var sessionId = $"session-{Guid.NewGuid():N}"; + await _traceRepo.AddAsync(MakeTrace(sessionId, 0, TraceKind.Procedure)); + await _traceRepo.AddAsync(MakeTrace(sessionId, 1, TraceKind.Procedure)); + var keptEpisodes = new[] + { + MakeTrace(sessionId, 2, TraceKind.Episode), + MakeTrace(sessionId, 3, TraceKind.Episode), + }; + foreach (var e in keptEpisodes) await _traceRepo.AddAsync(e); + + var pruned = await _traceRepo.PruneSessionTracesAsync(sessionId, maxToKeep: 2, ownerId: "alice"); + + pruned.Should().Be(0, "2 episodes with a cap of 2 is not over budget -- the procedures do not count"); + foreach (var e in keptEpisodes) + (await _traceRepo.GetByIdAsync(e.TraceId)).Should().NotBeNull(); + } + + [Fact] + public async Task TheExemption_DoesNotWeakenOwnerConfinement() + { + // The exemption clause was added to the WHERE of a destructive, session-keyed delete. That is + // exactly the edit where an owner guard gets weakened by accident, so the #40 cross-eviction + // property is re-asserted with procedures in the mix rather than assumed to still hold. + var sessionId = $"session-{Guid.NewGuid():N}"; + var bobProcedure = MakeTrace(sessionId, 0, TraceKind.Procedure, ownerId: "bob"); + var bobEpisode = MakeTrace(sessionId, 1, TraceKind.Episode, ownerId: "bob"); + await _traceRepo.AddAsync(bobProcedure); + await _traceRepo.AddAsync(bobEpisode); + for (var day = 0; day < 4; day++) + await _traceRepo.AddAsync(MakeTrace(sessionId, day, TraceKind.Episode, ownerId: "alice")); + + await _traceRepo.PruneSessionTracesAsync(sessionId, maxToKeep: 1, ownerId: "alice"); + + (await _traceRepo.GetByIdAsync(bobProcedure.TraceId)).Should().NotBeNull(); + (await _traceRepo.GetByIdAsync(bobEpisode.TraceId)).Should().NotBeNull( + "an owner-confined prune must never reach another owner's traces, whatever else it filters on"); + } +} diff --git a/tests/AgentMemory.Tests.Integration/Repositories/SupersededCandidateIntegrationTests.cs b/tests/AgentMemory.Tests.Integration/Repositories/SupersededCandidateIntegrationTests.cs new file mode 100644 index 00000000..59e2e625 --- /dev/null +++ b/tests/AgentMemory.Tests.Integration/Repositories/SupersededCandidateIntegrationTests.cs @@ -0,0 +1,154 @@ +using FluentAssertions; +using Microsoft.Extensions.Logging.Abstractions; +using AgentMemory.Abstractions.Domain; +using AgentMemory.Abstractions.Options; +using AgentMemory.Neo4j.Repositories; +using AgentMemory.Tests.Integration.Fixtures; + +namespace AgentMemory.Tests.Integration.Repositories; + +/// +/// The store side of write-time supersession (M1): which facts a new assertion replaces, live. +/// +/// +/// +/// The unit falsifier proves the decision — only functional relations, never the multi-valued +/// majority. This proves the selection, which is entirely Cypher and could not be checked with +/// a substitute: canonical-key matching, the liveness filter, owner confinement, and the exclusion of +/// the winner itself. +/// +/// +/// Liveness is the one that has to run here. invalidated_at is not carried on the domain +/// record, so an already-closed fact is indistinguishable from a live one to any in-memory filter — +/// and re-selecting closed facts would fan a supersession chain into a star, one new +/// :SUPERSEDED_BY edge per subsequent arrival. +/// +/// +[Collection("Neo4j Integration")] +[Trait("Category", "Integration")] +public class SupersededCandidateIntegrationTests : IAsyncLifetime +{ + private readonly Neo4jIntegrationFixture _fixture; + private readonly Neo4jFactRepository _facts; + + public SupersededCandidateIntegrationTests(Neo4jIntegrationFixture fixture) + { + _fixture = fixture; + _facts = new Neo4jFactRepository( + fixture.TransactionRunner, NullLogger.Instance); + } + + public Task InitializeAsync() => _fixture.CleanDatabaseAsync(); + public Task DisposeAsync() => Task.CompletedTask; + + private Task StoreAsync( + string subject, string predicate, string @object, string? ownerId = "alice") => + _facts.UpsertAsync(new Fact + { + FactId = $"fact-{Guid.NewGuid():N}", + Subject = subject, + Predicate = predicate, + Object = @object, + Confidence = 0.9, + OwnerId = ownerId, + CreatedAtUtc = DateTimeOffset.UtcNow, + }); + + private Task> CandidatesFor(Fact winner, bool scoped = true) => + _facts.FindSupersededCandidatesAsync( + winner.FactId, winner.Subject, winner.Predicate, winner.Object, + scoped ? MemoryScope.For("alice", includeShared: false) : null); + + [Fact] + public async Task TheEarlierAssertionIsSelected() + { + await StoreAsync("user", "lives in", "Basel"); + var winner = await StoreAsync("user", "lives in", "Zurich"); + + var candidates = await CandidatesFor(winner); + + candidates.Should().ContainSingle().Which.Object.Should().Be("Basel"); + } + + [Fact] + public async Task TheWinnerIsNeverItsOwnCandidate() + { + // A self-supersede would invalidate a live node and create a :SUPERSEDED_BY self-loop while + // reporting success. The Cypher rejects it, but the selection must not offer it either. + var winner = await StoreAsync("user", "lives in", "Zurich"); + + (await CandidatesFor(winner)).Should().BeEmpty(); + } + + [Fact] + public async Task ARestatementInDifferentWordsIsNotACandidate() + { + // Matching is on canonical keys, the same ones the write path MERGEs on. If casing or spacing + // produced a different key, a fact would supersede itself under a second identity. + await StoreAsync("User", "Lives In", "Zurich"); + var winner = await StoreAsync("user", "lives in", "zurich"); + + (await CandidatesFor(winner)).Should().BeEmpty(); + } + + [Fact] + public async Task AlreadySupersededFactsAreNotSelectedAgain() + { + // THE liveness property, and the reason this filter lives in Cypher. Without it, a third + // assertion would re-close the first -- harmless to its invalidated_at, which coalesce + // protects, but adding a second :SUPERSEDED_BY edge and turning a chain into a fan. + var first = await StoreAsync("user", "lives in", "Basel"); + var second = await StoreAsync("user", "lives in", "Bern"); + await _facts.SupersedeAsync(first.FactId, second.FactId, MemoryScope.For("alice", includeShared: false)); + + var third = await StoreAsync("user", "lives in", "Zurich"); + var candidates = await CandidatesFor(third); + + candidates.Should().ContainSingle().Which.FactId.Should().Be(second.FactId, + "only the live assertion is replaced; the one already closed stays closed"); + } + + [Fact] + public async Task ADifferentPredicateIsNotACandidate() + { + await StoreAsync("user", "works at", "Acme"); + var winner = await StoreAsync("user", "lives in", "Zurich"); + + (await CandidatesFor(winner)).Should().BeEmpty(); + } + + [Fact] + public async Task ADifferentSubjectIsNotACandidate() + { + await StoreAsync("bob", "lives in", "Basel"); + var winner = await StoreAsync("user", "lives in", "Zurich"); + + (await CandidatesFor(winner)).Should().BeEmpty(); + } + + [Fact] + public async Task AnotherOwnersFactIsNeverACandidate() + { + // Supersession closes a fact. Reaching across owners would close one belonging to somebody who + // was not in the conversation -- the worst shape this feature could take. + await StoreAsync("user", "lives in", "Basel", ownerId: "bob"); + var winner = await StoreAsync("user", "lives in", "Zurich"); + + (await CandidatesFor(winner)).Should().BeEmpty(); + } + + [Fact] + public async Task SupersessionIsNonDestructiveAndTheLoserRemainsReadable() + { + // "Fewer live facts" is only a win if nothing was lost. The loser keeps its content and its id; + // what changes is that it leaves live recall. + var loser = await StoreAsync("user", "lives in", "Basel"); + var winner = await StoreAsync("user", "lives in", "Zurich"); + + await _facts.SupersedeAsync(loser.FactId, winner.FactId, MemoryScope.For("alice", includeShared: false)); + + var stored = await _facts.GetByIdAsync(loser.FactId); + stored.Should().NotBeNull(); + stored!.Object.Should().Be("Basel"); + } +} diff --git a/tests/AgentMemory.Tests.Unit.LongMemEval/AblationTests.cs b/tests/AgentMemory.Tests.Unit.LongMemEval/AblationTests.cs new file mode 100644 index 00000000..ec89a3fd --- /dev/null +++ b/tests/AgentMemory.Tests.Unit.LongMemEval/AblationTests.cs @@ -0,0 +1,121 @@ +using AgentMemory.LongMemEval; +using FluentAssertions; +using Xunit; + +namespace AgentMemory.Tests.Unit.LongMemEval; + +/// +/// Ablating one capability and reporting which questions moved, not just an aggregate. +/// +/// +/// An aggregate hides the two things worth knowing: which questions a capability rescued, and whether +/// it broke any that previously worked. A capability that gains three and loses three reports as +/// neutral and is not — it is churn, and churn is the signature of a change moving noise rather than +/// adding signal. +/// +public sealed class AblationTests +{ + private static readonly Dictionary Types = new() + { + ["e1"] = "episodic", ["e2"] = "episodic", ["e3"] = "episodic", + ["s1"] = "semantic", ["s2"] = "semantic", + }; + + [Fact] + public void GainsAndLossesAreReportedSeparately() + { + var with = new Dictionary { ["e1"] = true, ["e2"] = false, ["s1"] = true }; + var without = new Dictionary { ["e1"] = false, ["e2"] = true, ["s1"] = true }; + + var result = LongMemEvalAblation.Compare( + LongMemEvalCapability.Episodic, with, without, Types); + + result.Gains.Should().ContainSingle().Which.QuestionId.Should().Be("e1"); + result.Losses.Should().ContainSingle().Which.QuestionId.Should().Be("e2"); + result.Net.Should().Be(0); + result.QuestionsCompared.Should().Be(3); + } + + [Fact] + public void ChurnIsVisibleEvenThoughTheNetIsZero() + { + // The case an aggregate hides entirely: three rescued, three broken, net zero. Reporting only + // the net would present a capability that is churning answers as one that does nothing. + var with = new Dictionary + { ["e1"] = true, ["e2"] = true, ["e3"] = true, ["s1"] = false, ["s2"] = false }; + var without = new Dictionary + { ["e1"] = false, ["e2"] = false, ["e3"] = false, ["s1"] = true, ["s2"] = true }; + + var result = LongMemEvalAblation.Compare( + LongMemEvalCapability.Episodic, with, without, Types); + + result.Net.Should().Be(1); + result.Gains.Should().HaveCount(3); + result.Losses.Should().HaveCount(2); + result.Flips.Should().HaveCount(5, "every moved question is listed, not just the balance"); + } + + [Fact] + public void AQuestionJudgedInOnlyOneArmIsNotCompared() + { + // Including it would let an arm that simply answered FEWER questions look like a capability + // effect -- an artefact that would be indistinguishable from a real gain in the aggregate. + var with = new Dictionary { ["e1"] = true, ["e2"] = true }; + var without = new Dictionary { ["e1"] = false }; + + var result = LongMemEvalAblation.Compare( + LongMemEvalCapability.Episodic, with, without, Types); + + result.QuestionsCompared.Should().Be(1); + result.Flips.Should().ContainSingle().Which.QuestionId.Should().Be("e1"); + } + + [Fact] + public void ANegativeResultIsRepresentable() + { + // A capability can make things worse, and that must be reportable rather than clamped. Finding + // it in-house is the whole point; the alternative is a competitor finding it. + var with = new Dictionary { ["e1"] = false, ["e2"] = false }; + var without = new Dictionary { ["e1"] = true, ["e2"] = true }; + + var result = LongMemEvalAblation.Compare( + LongMemEvalCapability.Episodic, with, without, Types); + + result.Net.Should().Be(-2); + result.NetPoints.Should().Be(-100); + } + + [Fact] + public void MovementInsideTheNoiseFloorIsNotAResult() + { + // 1 of 6 questions is 16.7 points. If episodic already varied by 33 points against ITSELF, + // that movement is noise -- and a null result is publishable, not a failure. + var with = new Dictionary + { ["e1"] = true, ["e2"] = true, ["e3"] = true, ["s1"] = true, ["s2"] = true }; + var without = new Dictionary + { ["e1"] = false, ["e2"] = true, ["e3"] = true, ["s1"] = true, ["s2"] = true }; + + var result = LongMemEvalAblation.Compare(LongMemEvalCapability.Episodic, with, without, Types); + var floors = LongMemEvalTypedNoiseFloorCalculator.Measure(new[] + { + new[] { new LongMemEvalTypedAccuracy("episodic", 6, 4, 0, 2, 0) }, + new[] { new LongMemEvalTypedAccuracy("episodic", 6, 2, 0, 4, 0) }, + }); + + LongMemEvalAblation.SurvivesNoiseFloor(result, floors).Should().BeFalse(); + } + + [Fact] + public void EveryCapabilityNamesTheOptionThatDisablesIt() + { + // A published ablation that cannot say which switch produced it is not reproducible. + foreach (var capability in LongMemEvalCapability.All) + { + capability.Option.Should().NotBeNullOrWhiteSpace(); + capability.MemoryType.Should().NotBeNullOrWhiteSpace(); + } + + LongMemEvalCapability.Episodic.MemoryType.Should().Be("episodic"); + LongMemEvalCapability.Traces.MemoryType.Should().Be("procedural"); + } +} diff --git a/tests/AgentMemory.Tests.Unit.LongMemEval/MemoryTypeSelectionTests.cs b/tests/AgentMemory.Tests.Unit.LongMemEval/MemoryTypeSelectionTests.cs new file mode 100644 index 00000000..bf454957 --- /dev/null +++ b/tests/AgentMemory.Tests.Unit.LongMemEval/MemoryTypeSelectionTests.cs @@ -0,0 +1,157 @@ +using AgentMemory.LongMemEval; +using FluentAssertions; +using Xunit; + +namespace AgentMemory.Tests.Unit.LongMemEval; + +/// +/// Selecting which memory types a run evaluates — the sampling side of the typed track. +/// +/// +/// +/// Per-type reporting already existed; per-type sampling did not, and the two are not +/// interchangeable. A 50-question stratified sample yields roughly 6 single-session-assistant +/// questions, so an episodic figure taken from it moves 16.7 points per item — while two runs of an +/// identical configuration have been measured 25 points apart on the full 50. Slicing a mixed sample +/// produces a per-type number that cannot distinguish a real effect from extraction nondeterminism. +/// +/// +/// ExternalBenchmarkOptions.IncludeQuestionTypes has been available since AgentEval 0.20 and +/// no caller passed it — a dead option, which is the same defect class this repo has hunted +/// before. These tests cover the wiring and, more importantly, the refusals. +/// +/// +public sealed class MemoryTypeSelectionTests +{ + [Fact] + public void NoSelectionMeansNoFilter() + { + // Null is the sampler's "everything" value. Every sealed base was recorded this way, so the + // unselected path must stay exactly what it was rather than becoming an explicit full list. + LongMemEvalMemoryTypeSelection.TaskTypesFor([]).Should().BeNull(); + } + + [Fact] + public void EpisodicSelectsTheAssistantTaskLabel() + { + // The concrete case Phase 8 needs: AssistantContentMode targets exactly the questions asking + // what the assistant said or did, and this is the label carrying them. + LongMemEvalMemoryTypeSelection.TaskTypesFor(["episodic"]) + .Should().BeEquivalentTo(["single-session-assistant"]); + } + + [Fact] + public void TemporalSelectsBothOfItsLabels() + { + // One memory type spans several task labels. Returning only the first would silently halve the + // sample while the run still reported itself as covering the type. + LongMemEvalMemoryTypeSelection.TaskTypesFor(["temporal"]) + .Should().BeEquivalentTo(["temporal-reasoning", "knowledge-update"]); + } + + [Fact] + public void SeveralTypesUnionTheirLabelsWithoutDuplicates() + { + var selected = LongMemEvalMemoryTypeSelection.TaskTypesFor(["episodic", "temporal"]); + + selected.Should().BeEquivalentTo( + ["single-session-assistant", "temporal-reasoning", "knowledge-update"]); + selected.Should().OnlyHaveUniqueItems(); + } + + [Fact] + public void SelectionIsCaseInsensitive() + { + LongMemEvalMemoryTypeSelection.TaskTypesFor(["Episodic"]) + .Should().BeEquivalentTo(["single-session-assistant"]); + } + + [Fact] + public void TheSameRequestAlwaysProducesTheSameOrder() + { + // The selection reaches a run fingerprint. An order that depended on dictionary enumeration + // would make two identical runs look like different configurations. + LongMemEvalMemoryTypeSelection.TaskTypesFor(["temporal", "episodic"]) + .Should().Equal(LongMemEvalMemoryTypeSelection.TaskTypesFor(["episodic", "temporal"])); + } + + // ── the refusals, which are the point ──────────────────────────────── + + [Fact] + public void AskingForProceduralIsRejectedRatherThanQuietlyIgnored() + { + // THE refusal. LongMemEval-S is chat QA: no build commands, no tool invocations, no fix + // trajectories. Returning an empty filter would widen the run back to every question and let + // the result be published as a procedural number -- the exact metric substitution the taxonomy + // file exists to prevent. + var act = () => LongMemEvalMemoryTypeSelection.TaskTypesFor(["procedural"]); + + act.Should().Throw() + .WithMessage("*procedural*"); + } + + [Fact] + public void AskingForMetamemoryIsRejectedAndPointsAtAbstention() + { + // Metamemory is real and this dataset does score it -- through abstention questions, which are + // selected by a different mechanism. The error has to say so, or it reads as "not supported". + var act = () => LongMemEvalMemoryTypeSelection.TaskTypesFor(["metamemory"]); + + act.Should().Throw() + .WithMessage("*abstention*"); + } + + [Fact] + public void AnUnknownTypeIsRejectedAndListsTheKnownOnes() + { + // A typo must not silently sample everything. Listing the known types is what separates + // "misspelled" from "not present in this dataset" for the reader. + var act = () => LongMemEvalMemoryTypeSelection.TaskTypesFor(["epsiodic"]); + + act.Should().Throw() + .Which.Message.Should().Contain("episodic"); + } + + [Fact] + public void OneBadTypeRejectsTheWholeRequest() + { + // Partial acceptance is the dangerous middle: the run would proceed, sample only the valid + // types, and report itself as covering both. + var act = () => LongMemEvalMemoryTypeSelection.TaskTypesFor(["episodic", "procedural"]); + + act.Should().Throw(); + } + + // ── consistency with the reporting side ────────────────────────────── + + [Fact] + public void EverySelectedLabelReportsTheTypeItWasSelectedFor() + { + // Selection and reporting must come from one taxonomy. If they ever diverged, a run would + // sample one set of labels and compute per-type accuracy from another -- and both halves would + // look internally correct, which is why this is asserted rather than assumed. + var map = LongMemEvalMemoryTypeMap.Default; + + foreach (var type in new[] { "semantic", "episodic", "temporal" }) + { + var labels = LongMemEvalMemoryTypeSelection.TaskTypesFor([type]); + labels.Should().NotBeNullOrEmpty(); + foreach (var label in labels!) + map.ForQuestion(label, isAbstention: false).Should().Contain(type, + $"'{label}' was selected for '{type}', so it must report as '{type}'"); + } + } + + [Fact] + public void EveryTaskLabelInTheDatasetIsReachableFromSomeType() + { + // A label no type selects is a question that can never appear in a typed run: it would vanish + // from typed measurement entirely while still counting in the aggregate. + var map = LongMemEvalMemoryTypeMap.Default; + var reachable = new[] { "semantic", "episodic", "temporal" } + .SelectMany(type => LongMemEvalMemoryTypeSelection.TaskTypesFor([type]) ?? []) + .ToHashSet(StringComparer.OrdinalIgnoreCase); + + map.TaskTypes.Keys.Should().BeSubsetOf(reachable); + } +} diff --git a/tests/AgentMemory.Tests.Unit.LongMemEval/SufficiencyAucTests.cs b/tests/AgentMemory.Tests.Unit.LongMemEval/SufficiencyAucTests.cs new file mode 100644 index 00000000..8a43201d --- /dev/null +++ b/tests/AgentMemory.Tests.Unit.LongMemEval/SufficiencyAucTests.cs @@ -0,0 +1,138 @@ +using AgentMemory.LongMemEval; +using FluentAssertions; +using Xunit; + +namespace AgentMemory.Tests.Unit.LongMemEval; + +/// +/// The instrument that decides whether the abstention track lives (PLAN 4.2). +/// +/// +/// +/// Per-section diagnostics say why a section came back thin. Everything downstream — calibration, +/// "I don't know", any confidence surface — assumes those numbers mean something. AUC is the +/// test of that assumption, and 0.5 is the kill line. +/// +/// +/// Which makes this instrument's own correctness load-bearing in an unusual way: an implementation +/// that reports a flattering number keeps a dead track alive for a quarter. So the tests below are +/// weighted towards the ways an AUC can be wrong while looking fine — ties, one empty class, and +/// inverted ordering. +/// +/// +public sealed class SufficiencyAucTests +{ + private static LongMemEvalSufficiencyAuc.Observation Present(double signal) => new(signal, true); + private static LongMemEvalSufficiencyAuc.Observation Absent(double signal) => new(signal, false); + + [Fact] + public void PerfectSeparationScoresOne() + { + var result = LongMemEvalSufficiencyAuc.Compute( + [Present(0.9), Present(0.8), Absent(0.2), Absent(0.1)]); + + result.Auc.Should().Be(1.0); + result.JustifiesAbstentionWork.Should().BeTrue(); + } + + [Fact] + public void PerfectlyInvertedSeparationScoresZero() + { + // Not symmetric with the above in consequence: an AUC of 0 means the signal is a perfect + // predictor read backwards, which is a wiring bug, not a dead signal. Reporting it as 1.0 -- + // by taking max(auc, 1-auc), a tempting "fix" -- would hide exactly that. + var result = LongMemEvalSufficiencyAuc.Compute( + [Present(0.1), Present(0.2), Absent(0.8), Absent(0.9)]); + + result.Auc.Should().Be(0.0); + result.JustifiesAbstentionWork.Should().BeFalse(); + } + + [Fact] + public void AConstantSignalScoresExactlyAHalf() + { + // THE case that matters. A signal returning the same value everywhere carries no information, + // and a curve-based implementation over a swept threshold can score such a tie block as if it + // were ordered. The rank form gives ties their 0.5 and reports the coin flip it is. + var result = LongMemEvalSufficiencyAuc.Compute( + [Present(0.5), Present(0.5), Absent(0.5), Absent(0.5)]); + + result.Auc.Should().Be(0.5); + result.JustifiesAbstentionWork.Should().BeFalse(); + result.TiedObservations.Should().BeGreaterThan(0); + } + + [Fact] + public void PartialTiesGetHalfCreditRatherThanFullCredit() + { + // One clean pair and one tied pair: 1 + 0.5 out of 2. + var result = LongMemEvalSufficiencyAuc.Compute( + [Present(0.9), Present(0.4), Absent(0.4)]); + + result.Auc.Should().BeApproximately(0.75, 1e-9); + } + + [Fact] + public void OneEmptyClassIsNotMeasuredRatherThanScoredAHalf() + { + // A run where every answer was present never asked the signal to order anything. Returning + // 0.5 would report "no signal" for a question that was not put -- and 0.5 is the kill line, so + // that particular default would kill a track on absent evidence. + var allPresent = LongMemEvalSufficiencyAuc.Compute([Present(0.9), Present(0.2)]); + + allPresent.Auc.Should().BeNull(); + allPresent.JustifiesAbstentionWork.Should().BeFalse(); + allPresent.Describe().Should().Contain("not measured"); + } + + [Fact] + public void NoObservationsAtAllIsAlsoNotMeasured() + { + LongMemEvalSufficiencyAuc.Compute([]).Auc.Should().BeNull(); + } + + [Fact] + public void TheDescriptionAlwaysStatesBothDenominators() + { + // An AUC without its class counts is unreadable: 0.83 over 47 present and 3 absent is three + // questions' worth of evidence, and the bare number hides that entirely. + var result = LongMemEvalSufficiencyAuc.Compute( + [Present(0.9), Present(0.8), Present(0.7), Absent(0.1)]); + + result.Describe().Should().Contain("3 present").And.Contain("1 absent"); + } + + [Fact] + public void TheJustificationThresholdIsFixedInAdvanceAndSitsAboveTheCoinLine() + { + // Stated before any number is seen, so the conclusion cannot be chosen after the fact. + LongMemEvalSufficiencyAuc.Compute([Present(0.9), Absent(0.1)]) + .JustifiesAbstentionWork.Should().BeTrue(); + + // 0.5 exactly -- a coin -- must not justify anything. + LongMemEvalSufficiencyAuc.Compute([Present(0.5), Absent(0.5)]) + .JustifiesAbstentionWork.Should().BeFalse(); + } + + [Fact] + public void OrderOfObservationsDoesNotChangeTheResult() + { + // Rank computation over an unsorted input is where an off-by-one hides, and it would produce a + // number that is wrong by a little -- the hardest kind to notice. + var forward = LongMemEvalSufficiencyAuc.Compute( + [Present(0.9), Absent(0.3), Present(0.5), Absent(0.7)]); + var reversed = LongMemEvalSufficiencyAuc.Compute( + [Absent(0.7), Present(0.5), Absent(0.3), Present(0.9)]); + + forward.Auc.Should().Be(reversed.Auc); + } + + [Fact] + public void AKnownHandComputedCaseMatches() + { + // Present {0.9, 0.5}, absent {0.7, 0.3}: pairs (0.9>0.7)=1, (0.9>0.3)=1, (0.5<0.7)=0, + // (0.5>0.3)=1 -> 3/4. + LongMemEvalSufficiencyAuc.Compute([Present(0.9), Present(0.5), Absent(0.7), Absent(0.3)]) + .Auc.Should().BeApproximately(0.75, 1e-9); + } +} diff --git a/tests/AgentMemory.Tests.Unit.LongMemEval/TypedNoiseFloorTests.cs b/tests/AgentMemory.Tests.Unit.LongMemEval/TypedNoiseFloorTests.cs new file mode 100644 index 00000000..8ca3f9cc --- /dev/null +++ b/tests/AgentMemory.Tests.Unit.LongMemEval/TypedNoiseFloorTests.cs @@ -0,0 +1,107 @@ +using AgentMemory.LongMemEval; +using FluentAssertions; +using Xunit; + +namespace AgentMemory.Tests.Unit.LongMemEval; + +/// +/// The per-type band that has to exist before any per-type number is published. +/// +/// +/// A per-type subset is small — episodic is 6 of 50 questions, i.e. 16.7 accuracy points per +/// question — and the whole-run band (~±9 points at n=50) does not transfer to it. Quoting a +/// per-type figure without its own band is how a table invites exactly the comparison it cannot +/// support. +/// +public sealed class TypedNoiseFloorTests +{ + private static LongMemEvalTypedAccuracy Row(string type, int questions, int correct) => + new(type, questions, correct, 0, questions - correct, 0); + + [Fact] + public void SpreadIsMeasuredAcrossRepeatsOfTheSameArm() + { + var runs = new[] + { + new[] { Row("episodic", 6, 4) }, // 66.7% + new[] { Row("episodic", 6, 3) }, // 50.0% + new[] { Row("episodic", 6, 5) }, // 83.3% + }; + + var floor = LongMemEvalTypedNoiseFloorCalculator.Measure(runs).Single(); + + floor.Runs.Should().Be(3); + floor.Questions.Should().Be(6); + floor.RangePoints.Should().BeApproximately(33.3, 0.1); + floor.PointsPerQuestion.Should().BeApproximately(16.67, 0.01); + floor.StandardDeviation.Should().NotBeNull(); + } + + [Fact] + public void ADifferenceInsideTheObservedSpreadDoesNotSeparate() + { + // The load-bearing guard. If one configuration already varied by 33 points against ITSELF, a + // 20-point difference against another configuration is not a result -- and reporting it as one + // is precisely how this category produces numbers that dissolve under scrutiny. + var runs = new[] + { + new[] { Row("episodic", 6, 4) }, + new[] { Row("episodic", 6, 3) }, + new[] { Row("episodic", 6, 5) }, + }; + + var floor = LongMemEvalTypedNoiseFloorCalculator.Measure(runs).Single(); + + floor.Separates(20.0).Should().BeFalse("20 points is inside a 33-point self-spread"); + floor.Separates(40.0).Should().BeTrue(); + } + + [Fact] + public void ASingleRunSeparatesNothing() + { + // One run yields no spread, so nothing is separable. That is the correct answer, not a missing + // feature -- and it is why a first-ever per-type table cannot carry a comparative claim. + var floor = LongMemEvalTypedNoiseFloorCalculator + .Measure(new[] { new[] { Row("semantic", 23, 20) } }).Single(); + + floor.Runs.Should().Be(1); + floor.StandardDeviation.Should().BeNull(); + floor.Separates(0.1).Should().BeFalse(); + floor.Separates(99.0).Should().BeFalse("with one run there is no observed variation to beat"); + } + + [Fact] + public void PerfectlyStableRepeatsReportZeroSpread() + { + // What a deterministic extractor would look like. Worth being able to SEE, because it is the + // whole prize of a temperature-0 deployment: a zero band means one build per arm suffices. + var runs = Enumerable.Repeat(new[] { Row("temporal", 21, 21) }, 3).ToArray(); + + var floor = LongMemEvalTypedNoiseFloorCalculator.Measure(runs).Single(); + + floor.RangePoints.Should().Be(0); + floor.StandardDeviation.Should().Be(0); + floor.Separates(0.5).Should().BeTrue("with no observed variation, any real difference separates"); + } + + [Fact] + public void TypesAreMeasuredIndependently() + { + var runs = new[] + { + new[] { Row("semantic", 23, 20), Row("episodic", 6, 4) }, + new[] { Row("semantic", 23, 20), Row("episodic", 6, 2) }, + }; + + var floors = LongMemEvalTypedNoiseFloorCalculator.Measure(runs); + + floors.Single(f => f.MemoryType == "semantic").RangePoints.Should().Be(0); + floors.Single(f => f.MemoryType == "episodic").RangePoints.Should().BeApproximately(33.3, 0.1); + } + + [Fact] + public void NoRunsYieldsNoBands() + { + LongMemEvalTypedNoiseFloorCalculator.Measure([]).Should().BeEmpty(); + } +} diff --git a/tests/AgentMemory.Tests.Unit.LongMemEval/TypedReportTests.cs b/tests/AgentMemory.Tests.Unit.LongMemEval/TypedReportTests.cs new file mode 100644 index 00000000..52889789 --- /dev/null +++ b/tests/AgentMemory.Tests.Unit.LongMemEval/TypedReportTests.cs @@ -0,0 +1,114 @@ +using AgentMemory.LongMemEval; +using FluentAssertions; +using Xunit; + +namespace AgentMemory.Tests.Unit.LongMemEval; + +/// +/// The integrity rules, enforced in the renderer rather than left to whoever writes the prose. +/// +/// +/// The failure mode guarded against is a number that travels without its context. Assume every caveat +/// is stripped and the bare figure is quoted — so the figure itself has to be safe. +/// +public sealed class TypedReportTests +{ + private static readonly LongMemEvalTypedAccuracy Episodic = new("episodic", 6, 4, 0, 2, 0); + private static readonly LongMemEvalTypedAccuracy Semantic = new("semantic", 23, 20, 0, 2, 1); + + [Fact] + public void ASingleRunIsLabelledAsHavingNoBand() + { + // The most dangerous artefact this project could produce: a per-type table from ONE run that + // reads as though it supports comparison. + var report = LongMemEvalTypedReport.Render([Episodic, Semantic]); + + report.Should().Contain("not measured"); + report.Should().Contain("No band has been measured"); + report.Should().Contain("none of them supports a comparison"); + } + + [Fact] + public void EveryRowCarriesItsQuestionCountAndWhatOneQuestionIsWorth() + { + var report = LongMemEvalTypedReport.Render([Episodic]); + + report.Should().Contain("| episodic | 6 |"); + report.Should().Contain("16.7 pts", "six questions is 16.7 accuracy points each, and a reader must see that"); + } + + [Fact] + public void AnAblationInsideTheBandIsPrintedAsNoResult() + { + // Not as a small win. A component that does not earn its tokens should be found here. + var floors = LongMemEvalTypedNoiseFloorCalculator.Measure(new[] + { + new[] { new LongMemEvalTypedAccuracy("episodic", 6, 4, 0, 2, 0) }, + new[] { new LongMemEvalTypedAccuracy("episodic", 6, 2, 0, 4, 0) }, + }); + var ablation = new LongMemEvalAblationResult( + LongMemEvalCapability.Episodic, + [new LongMemEvalQuestionFlip("e1", "episodic", true, false)], + 6); + + var report = LongMemEvalTypedReport.Render([Episodic], floors, [ablation]); + + report.Should().Contain("no result — inside the band"); + } + + [Fact] + public void ChurnIsCalledOutEvenWhenTheNetLooksFine() + { + var ablation = new LongMemEvalAblationResult( + LongMemEvalCapability.Episodic, + [ + new LongMemEvalQuestionFlip("e1", "episodic", true, false), + new LongMemEvalQuestionFlip("e2", "episodic", true, false), + new LongMemEvalQuestionFlip("e3", "episodic", false, true), + ], + 6); + + var report = LongMemEvalTypedReport.Render([Episodic], null, [ablation]); + + report.Should().Contain("Churn:"); + report.Should().Contain("moving answers"); + } + + [Fact] + public void TheUnreachableTypesAreAlwaysStated() + { + // A missing row must never read as a zero. Procedural is absent because the dataset cannot + // reach it, not because we scored badly. + var report = LongMemEvalTypedReport.Render([Semantic]); + + report.Should().Contain("Procedural memory is unreachable here at any sample size"); + report.Should().Contain("a missing row means unmeasured, never zero"); + } + + [Fact] + public void TheMappingRevisionIsPrintedWhenSupplied() + { + var report = LongMemEvalTypedReport.Render([Semantic], mappingRevision: "2026-08-12"); + + report.Should().Contain("2026-08-12"); + report.Should().Contain("The grouping is an opinion"); + } + + [Fact] + public void AMeasuredBandReplacesTheNotMeasuredLabel() + { + var floors = LongMemEvalTypedNoiseFloorCalculator.Measure(new[] + { + new[] { new LongMemEvalTypedAccuracy("episodic", 6, 4, 0, 2, 0) }, + new[] { new LongMemEvalTypedAccuracy("episodic", 6, 3, 0, 3, 0) }, + }); + + var report = LongMemEvalTypedReport.Render([Episodic], floors); + + report.Should().Contain("±"); + // The BAND CELL specifically, not the prose: the closing section legitimately says a missing + // row means "unmeasured", and a broader assertion here failed against correct output. + report.Should().NotContain("**not measured**"); + report.Should().NotContain("No band has been measured"); + } +} diff --git a/tests/AgentMemory.Tests.Unit/AgentMemory.Tests.Unit.csproj b/tests/AgentMemory.Tests.Unit/AgentMemory.Tests.Unit.csproj index 8f9fd77d..2ca9416b 100644 --- a/tests/AgentMemory.Tests.Unit/AgentMemory.Tests.Unit.csproj +++ b/tests/AgentMemory.Tests.Unit/AgentMemory.Tests.Unit.csproj @@ -35,6 +35,9 @@ + + diff --git a/tests/AgentMemory.Tests.Unit/Cli/CliCommandsTests.cs b/tests/AgentMemory.Tests.Unit/Cli/CliCommandsTests.cs index fa42af7b..04067942 100644 --- a/tests/AgentMemory.Tests.Unit/Cli/CliCommandsTests.cs +++ b/tests/AgentMemory.Tests.Unit/Cli/CliCommandsTests.cs @@ -90,8 +90,8 @@ public async Task SchemaCheckCommand_OwnedIndexFailed_ReturnsOne_EvenThoughEvery var runner = Substitute.For(); runner.ReadAsync(Arg.Any>>>(), Arg.Any()) .Returns(present); - runner.ReadAsync(Arg.Any>>(), Arg.Any()) - .Returns([$"{broken} (RANGE)"]); + runner.ReadAsync(Arg.Any>>(), Arg.Any()) + .Returns([new IndexState(broken, "FAILED", "RANGE", null)]); var exit = await new SchemaCheckCommand(runner, options, _output).ExecuteAsync(); @@ -112,8 +112,8 @@ public async Task SchemaCheckCommand_ForeignIndexFailed_StillReturnsZero_ButSays var runner = Substitute.For(); runner.ReadAsync(Arg.Any>>>(), Arg.Any()) .Returns(present); - runner.ReadAsync(Arg.Any>>(), Arg.Any()) - .Returns(["someone_elses_idx (RANGE)"]); + runner.ReadAsync(Arg.Any>>(), Arg.Any()) + .Returns([new IndexState("someone_elses_idx", "FAILED", "RANGE", null)]); var exit = await new SchemaCheckCommand(runner, options, _output).ExecuteAsync(); @@ -121,6 +121,37 @@ public async Task SchemaCheckCommand_ForeignIndexFailed_StillReturnsZero_ButSays _output.ToString().Should().Contain("someone_elses_idx").And.Contain("not created by AgentMemory"); } + /// + /// P6. A POPULATING index is neither healthy nor failed, and it was the state this command could + /// not describe at all. + /// + /// + /// It matters most on the vector indexes: a search against a half-built one succeeds and returns a + /// subset of the corpus, so recall is quietly partial and the symptom is "memory seems to + /// have forgotten things" rather than any error. Transient, so it must not fail the check -- but + /// silence is how an operator spends an afternoon debugging retrieval quality on a half-built + /// index. + /// + [Fact] + public async Task SchemaCheckCommand_PopulatingIndex_ReturnsZero_ButSaysWhatItMeans() + { + var options = Options.Create(new Neo4jOptions { EmbeddingDimensions = 1536, Database = "neo4j" }); + var present = new HashSet(SchemaConformance.ExpectedObjectNames(1536), StringComparer.Ordinal); + var building = SchemaConformance.ExpectedObjectNames(1536)[0]; + var runner = Substitute.For(); + runner.ReadAsync(Arg.Any>>>(), Arg.Any()) + .Returns(present); + runner.ReadAsync(Arg.Any>>(), Arg.Any()) + .Returns([new IndexState(building, "POPULATING", "VECTOR", 42.5)]); + + var exit = await new SchemaCheckCommand(runner, options, _output).ExecuteAsync(); + + exit.Should().Be(0, "populating is transient and legitimate right after bootstrap"); + _output.ToString().Should().Contain("POPULATING").And.Contain(building) + .And.Contain("42.5", "the percentage is the difference between 'wait' and 'something is wrong'") + .And.Contain("subset", "an operator must be told WHY a half-built index matters"); + } + [Fact] public async Task ConsolidateCommand_DefaultsToDryRun() { diff --git a/tests/AgentMemory.Tests.Unit/Extraction/ExtractorProjectionConformanceTests.cs b/tests/AgentMemory.Tests.Unit/Extraction/ExtractorProjectionConformanceTests.cs new file mode 100644 index 00000000..f39160cf --- /dev/null +++ b/tests/AgentMemory.Tests.Unit/Extraction/ExtractorProjectionConformanceTests.cs @@ -0,0 +1,193 @@ +using AgentMemory.Abstractions.Domain; +using AgentMemory.Abstractions.Options; +using AgentMemory.Extraction.Llm; +using FluentAssertions; +using Microsoft.Extensions.AI; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; +using NSubstitute; +using Xunit; + +namespace AgentMemory.Tests.Unit.Extraction; + +/// +/// A field a rung asks for must survive that rung's projection into the domain record. +/// +/// +/// +/// The existing conformance tests check that every rung's prompt carries a setting's +/// instruction. That is only half the contract, and the missing half had already failed silently: the +/// multi-session batch rung asked for valid_from/valid_until whenever +/// TemporalValidityMode.Extract was set — the instruction is shared, so it could not not ask — +/// and then dropped both fields on the floor when building its . The +/// setting was a no-op under batched extraction, and the prompt-level test passed the whole time. +/// +/// +/// This is the same "a setting only some extractors respect" defect the shared-semantics type was +/// created to prevent, arriving one layer lower than anyone was looking. So the rule is asserted where +/// it can actually be broken: parse a response that populates the field, and require the value to come +/// out the other end. +/// +/// +public sealed class ExtractorProjectionConformanceTests +{ + private static readonly DateTimeOffset ValidFrom = new(2026, 3, 1, 0, 0, 0, TimeSpan.Zero); + private static readonly DateTimeOffset ValidUntil = new(2026, 9, 1, 0, 0, 0, TimeSpan.Zero); + + private static IChatClient ClientReturning(string payload) + { + var client = Substitute.For(); + client.GetResponseAsync( + Arg.Any>(), Arg.Any(), Arg.Any()) + .Returns(_ => Task.FromResult( + new ChatResponse(new ChatMessage(ChatRole.Assistant, payload)))); + return client; + } + + private static IReadOnlyList OneMessage() => + [ + new Message + { + MessageId = "m-1", + ConversationId = "c-1", + SessionId = "s-1", + Role = "user", + Content = "I am on the Zurich project until September.", + TimestampUtc = ValidFrom, + }, + ]; + + private const string FactFields = + "\"subject\":\"user\",\"predicate\":\"works on\",\"object\":\"Zurich project\",\"confidence\":0.9," + + "\"valid_from\":\"2026-03-01T00:00:00+00:00\",\"valid_until\":\"2026-09-01T00:00:00+00:00\"," + + "\"source_role\":\"assistant\",\"source_turn\":2"; + + private const string PreferenceFields = + "\"category\":\"travel\",\"preference\":\"aisle seats\",\"confidence\":0.9," + + "\"source_role\":\"assistant\",\"source_turn\":2"; + + // ── the unified rung ────────────────────────────────────────────────── + + [Fact] + public async Task TheUnifiedRungCarriesEveryRequestedFieldThrough() + { + var payload = + $"{{\"entities\":[],\"facts\":[{{{FactFields}}}]," + + $"\"preferences\":[{{{PreferenceFields}}}],\"relations\":[]}}"; + var sut = new LlmUnifiedMemoryExtractor( + ClientReturning(payload), + Options.Create(new LlmExtractionOptions { UseUnifiedExtraction = true, MaxRetries = 0 }), + NullLogger.Instance); + + var result = await sut.ExtractAsync(OneMessage()); + + var fact = result.Facts.Should().ContainSingle().Subject; + fact.ValidFrom.Should().Be(ValidFrom); + fact.ValidUntil.Should().Be(ValidUntil); + fact.SourceRole.Should().Be("assistant"); + fact.SourceTurn.Should().Be(2); + var preference = result.Preferences.Should().ContainSingle().Subject; + preference.SourceRole.Should().Be("assistant"); + preference.SourceTurn.Should().Be(2); + } + + // ── the multi-session batch rung — where the defect was ─────────────── + + [Fact] + public async Task TheBatchRungCarriesEveryRequestedFieldThrough() + { + // The regression that shipped: this rung emitted the temporal instruction and then discarded + // the answer, so TemporalValidityMode.Extract was silently inert under batched extraction. + var alias = LlmMultiSessionExtractionResponseContract.Alias(0); + var payload = + $"{{\"processed_source_sessions\":[\"{alias}\"],\"entities\":[]," + + $"\"facts\":[{{\"source_session\":\"{alias}\",{FactFields}}}]," + + $"\"preferences\":[{{\"source_session\":\"{alias}\",{PreferenceFields}}}],\"relations\":[]}}"; + + var options = Options.Create(new LlmExtractionOptions + { + UseUnifiedExtraction = true, + UseMultiSessionBatchExtraction = true, + MaxRetries = 0, + }); + var sut = new LlmMultiSessionUnifiedMemoryExtractor( + ClientReturning(payload), + options, + NullLogger.Instance, + new LlmExtractionBatchConcurrencyLimiter(options)); + + var results = await sut.ExtractAsync( + [new ExtractionRequest { SessionId = "s-1", UserId = "owner-1", Messages = OneMessage() }], + maxSessionsPerBatch: 1, + maxInputTokens: 100_000); + + var extracted = results["s-1"]; + var fact = extracted.Facts.Should().ContainSingle().Subject; + fact.ValidFrom.Should().Be(ValidFrom, + "this rung asks for valid_from whenever Extract is set, so dropping it makes the setting a no-op"); + fact.ValidUntil.Should().Be(ValidUntil); + fact.SourceRole.Should().Be("assistant"); + fact.SourceTurn.Should().Be(2); + var preference = extracted.Preferences.Should().ContainSingle().Subject; + preference.SourceRole.Should().Be("assistant"); + preference.SourceTurn.Should().Be(2); + } + + // ── the per-kind rungs ──────────────────────────────────────────────── + + [Fact] + public async Task ThePerKindFactRungCarriesEveryRequestedFieldThrough() + { + var sut = new LlmFactExtractor( + ClientReturning($"{{\"facts\":[{{{FactFields}}}]}}"), + Options.Create(new LlmExtractionOptions { MaxRetries = 0 }), + NullLogger.Instance); + + var facts = await sut.ExtractAsync(OneMessage()); + + var fact = facts.Should().ContainSingle().Subject; + fact.ValidFrom.Should().Be(ValidFrom); + fact.ValidUntil.Should().Be(ValidUntil); + fact.SourceRole.Should().Be("assistant"); + fact.SourceTurn.Should().Be(2); + } + + [Fact] + public async Task ThePerKindPreferenceRungCarriesEveryRequestedFieldThrough() + { + var sut = new LlmPreferenceExtractor( + ClientReturning($"{{\"preferences\":[{{{PreferenceFields}}}]}}"), + Options.Create(new LlmExtractionOptions { MaxRetries = 0 }), + NullLogger.Instance); + + var preferences = await sut.ExtractAsync(OneMessage()); + + var preference = preferences.Should().ContainSingle().Subject; + preference.SourceRole.Should().Be("assistant"); + preference.SourceTurn.Should().Be(2); + } + + // ── and the absence direction ───────────────────────────────────────── + + [Fact] + public async Task AResponseThatOmitsTheFieldsProducesNullsRatherThanDefaults() + { + // Null is the meaningful value on all three: an invented valid_until silently removes a memory + // from every future answer, and an invented source_role moves a trust stamp. A projection that + // substituted "now" or "user" for a missing field would be worse than one that dropped it. + var payload = + "{\"entities\":[],\"facts\":[{\"subject\":\"user\",\"predicate\":\"likes\"," + + "\"object\":\"tea\",\"confidence\":0.9}],\"preferences\":[],\"relations\":[]}"; + var sut = new LlmUnifiedMemoryExtractor( + ClientReturning(payload), + Options.Create(new LlmExtractionOptions { UseUnifiedExtraction = true, MaxRetries = 0 }), + NullLogger.Instance); + + var fact = (await sut.ExtractAsync(OneMessage())).Facts.Should().ContainSingle().Subject; + + fact.ValidFrom.Should().BeNull(); + fact.ValidUntil.Should().BeNull(); + fact.SourceRole.Should().BeNull(); + fact.SourceTurn.Should().BeNull(); + } +} diff --git a/tests/AgentMemory.Tests.Unit/Extraction/PerItemProvenanceTests.cs b/tests/AgentMemory.Tests.Unit/Extraction/PerItemProvenanceTests.cs new file mode 100644 index 00000000..b6ab6e4f --- /dev/null +++ b/tests/AgentMemory.Tests.Unit/Extraction/PerItemProvenanceTests.cs @@ -0,0 +1,310 @@ +using FluentAssertions; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; +using AgentMemory.Abstractions.Domain; +using AgentMemory.Abstractions.Options; +using AgentMemory.Abstractions.Repositories; +using AgentMemory.Abstractions.Services; +using AgentMemory.Core.Extraction; +using AgentMemory.Core.Services; +using AgentMemory.Extraction.Llm; +using NSubstitute; +using Xunit; + +namespace AgentMemory.Tests.Unit.Extraction; + +/// +/// Per-item provenance (L3c): a stored fact must be attributable to the turn that stated it. +/// +/// +/// +/// EXTRACTED_FROM is written per ingestion batch — every item linked to every message the call +/// saw. On the evaluation corpus a fact links to a mean of 12 source messages and as many as +/// 30. The consequence is not merely imprecision: any attribution metric derived from that edge is +/// satisfied by construction. "Is the true source among this fact's linked messages?" is yes for +/// all thirty, so the metric cannot fail, and a provenance regression would be invisible to it. +/// +/// +/// So the assertions below are the ones that can fail — a narrowed link is asserted to name the +/// right turn, not merely to be short, and the fallbacks are asserted to keep the batch rather than +/// silently attribute a fact to whichever message happens to sit at a hallucinated index. +/// +/// +public sealed class PerItemProvenanceTests +{ + private readonly IEmbeddingOrchestrator _orchestrator = Substitute.For(); + private readonly IEntityRepository _entityRepo = Substitute.For(); + private readonly IFactRepository _factRepo = Substitute.For(); + private readonly IPreferenceRepository _prefRepo = Substitute.For(); + private readonly IRelationshipRepository _relRepo = Substitute.For(); + private readonly IClock _clock = Substitute.For(); + private readonly IIdGenerator _idGen = Substitute.For(); + + private readonly List _writtenFacts = []; + private readonly List _writtenPreferences = []; + private readonly List<(string Id, string MessageId)> _factEdges = []; + + private static readonly IReadOnlyList FiveMessages = + ["msg-1", "msg-2", "msg-3", "msg-4", "msg-5"]; + + public PerItemProvenanceTests() + { + _clock.UtcNow.Returns(DateTimeOffset.UtcNow); + _idGen.GenerateId().Returns(_ => Guid.NewGuid().ToString("N")); + _orchestrator.EmbedAsync(Arg.Any(), Arg.Any()).Returns(new float[8]); + + _factRepo.UpsertAsync(Arg.Any(), Arg.Any()) + .Returns(ci => + { + var fact = ci.Arg(); + _writtenFacts.Add(fact); + // A MERGE returns the STORED node, whose source ids are the union accumulated over + // earlier ingestions. Returning a deliberately different list here is what catches an + // implementation that writes edges from the repository result instead of the input. + return Task.FromResult(fact with { SourceMessageIds = FiveMessages }); + }); + _factRepo.CreateExtractedFromRelationshipAsync( + Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(ci => + { + _factEdges.Add((ci.ArgAt(0), ci.ArgAt(1))); + return Task.CompletedTask; + }); + _prefRepo.UpsertAsync(Arg.Any(), Arg.Any()) + .Returns(ci => + { + _writtenPreferences.Add(ci.Arg()); + return Task.FromResult(ci.Arg()); + }); + } + + private PersistenceStage CreateSut() => + new(_orchestrator, _entityRepo, _factRepo, _prefRepo, _relRepo, _clock, _idGen, + NullLogger.Instance, + new PassThroughMemoryPersistenceTransaction(), Options.Create(new ExtractionOptions())); + + private static ExtractionStageResult WithFact(int? sourceTurn) => new() + { + FilteredFacts = + [ + new ExtractedFact + { + Subject = "user", Predicate = "lives in", Object = "Zurich", + Confidence = 0.9, SourceTurn = sourceTurn, + }, + ], + SourceMessageIds = FiveMessages, + }; + + // ── the narrowing ───────────────────────────────────────────────────── + + [Fact] + public async Task AReportedTurnNarrowsTheFactToThatOneMessage() + { + await CreateSut().PersistAsync(WithFact(sourceTurn: 3)); + + // Turn N is messages[N-1]: the transcript is numbered from the same ordered list the source + // ids come from, so this is a positional index rather than a search. + _writtenFacts.Should().ContainSingle().Which.SourceMessageIds.Should().Equal(["msg-3"]); + } + + [Fact] + public async Task TheExtractedFromEdgeIsWrittenForThatMessageOnly() + { + // The stored property and the edge are two separate writes. Narrowing one while leaving the + // other at batch breadth would leave the graph exactly as unattributable as before. + await CreateSut().PersistAsync(WithFact(sourceTurn: 3)); + + _factEdges.Select(edge => edge.MessageId).Should().Equal(["msg-3"]); + } + + [Theory] + [InlineData(1, "msg-1")] + [InlineData(5, "msg-5")] + public async Task TheBoundaryTurnsResolveToTheBoundaryMessages(int turn, string expected) + { + // Off-by-one here would attribute every fact to its neighbour, which reads as plausible + // provenance forever after. + await CreateSut().PersistAsync(WithFact(turn)); + + _writtenFacts.Should().ContainSingle().Which.SourceMessageIds.Should().Equal([expected]); + } + + // ── the fallbacks, which matter more ────────────────────────────────── + + [Fact] + public async Task NoReportedTurnKeepsTheBatchLinksExactlyAsBefore() + { + // The byte-identical guarantee at defaults: ExtractionProvenanceMode.Batch never populates + // SourceTurn, so every item takes this path and provenance is what it always was. + await CreateSut().PersistAsync(WithFact(sourceTurn: null)); + + _writtenFacts.Should().ContainSingle().Which.SourceMessageIds.Should().Equal(FiveMessages); + _factEdges.Select(edge => edge.MessageId).Should().Equal(FiveMessages); + } + + [Theory] + [InlineData(0)] + [InlineData(-1)] + [InlineData(6)] + [InlineData(int.MaxValue)] + public async Task AnUnusableTurnFallsBackRatherThanAttributingWrongly(int turn) + { + // THE safety property. A resolved turn REPLACES the batch links, so a guessed number does not + // add noise -- it discards the true source and substitutes a wrong one, and afterwards the + // result is indistinguishable from precise attribution. Coarse provenance is recoverable; + // confidently wrong provenance is not. Clamping to the nearest valid index would be the + // tempting bug: it always produces an answer, and the answer is fabricated. + await CreateSut().PersistAsync(WithFact(turn)); + + _writtenFacts.Should().ContainSingle().Which.SourceMessageIds.Should().Equal(FiveMessages); + } + + [Fact] + public async Task EdgesComeFromTheInputItemNotTheMergedRepositoryResult() + { + // The fact repository here returns a stored node whose source ids are the full batch, mimicking + // a MERGE against an earlier ingestion. Writing edges from that result would re-link this fact + // to messages it was not extracted from -- restoring the exact breadth this removes, while the + // stored property still looked correct. + await CreateSut().PersistAsync(WithFact(sourceTurn: 2)); + + _factEdges.Select(edge => edge.MessageId).Should().Equal(["msg-2"]); + } + + [Fact] + public async Task PreferencesNarrowToo() + { + var extraction = new ExtractionStageResult + { + FilteredPreferences = + [ + new ExtractedPreference + { + Category = "travel", PreferenceText = "aisle seats", + Confidence = 0.9, SourceTurn = 4, + }, + ], + SourceMessageIds = FiveMessages, + }; + + await CreateSut().PersistAsync(extraction); + + _writtenPreferences.Should().ContainSingle().Which.SourceMessageIds.Should().Equal(["msg-4"]); + } + + // ── the resolver's own contract ─────────────────────────────────────── + + [Fact] + public void NarrowingIsReportedOnlyWhenItActuallyHappened() + { + // A resolver that never fires is indistinguishable from one that always does if the only + // signal is "the fact has source ids". Single-message batches are not a narrowing: there was + // nothing to narrow. + SourceTurnProvenance.Narrowed(3, FiveMessages).Should().BeTrue(); + SourceTurnProvenance.Narrowed(null, FiveMessages).Should().BeFalse(); + SourceTurnProvenance.Narrowed(9, FiveMessages).Should().BeFalse(); + SourceTurnProvenance.Narrowed(1, ["msg-1"]).Should().BeFalse(); + } + + // ── the prompt and transcript, which must move together ─────────────── + + [Fact] + public void TheDefaultModeAddsNothingToThePrompt() + { + ExtractionPromptSemantics.ProvenanceInstruction(ExtractionProvenanceMode.Batch) + .Should().BeEmpty(); + } + + [Fact] + public void PerItemAsksForOneTurnAndForbidsGuessing() + { + var instruction = ExtractionPromptSemantics.ProvenanceInstruction( + ExtractionProvenanceMode.PerItem); + + instruction.Should().Contain("source_turn"); + // Load-bearing: a resolved turn replaces the batch links, so an invented number is worse than + // no number at all. + instruction.Should().MatchRegex("(?i)(unsure|never guess)"); + } + + [Theory] + [InlineData(ExtractionProvenanceMode.PerItem)] + public void EveryExtractorRungCarriesTheInstruction(ExtractionProvenanceMode mode) + { + // The three rungs are meant to be interchangeable, and each rewrote its prompt from scratch. + // A setting only some of them honour makes behaviour depend on a performance flag -- which is + // how TemporalValidityMode ended up inert on the batch rung. + var expected = ExtractionPromptSemantics.ProvenanceInstruction(mode); + + var prompts = new (string Rung, string Prompt)[] + { + ("per-kind fact", LlmFactExtractor.BuildSystemPrompt( + AssistantContentMode.Ignore, TemporalValidityMode.Ignore, mode)), + ("unified", LlmUnifiedMemoryExtractor.BuildSystemPrompt( + AssistantContentMode.Ignore, [], TemporalValidityMode.Ignore, mode)), + ("multi-session batch", LlmMultiSessionUnifiedMemoryExtractor.BuildSystemPrompt( + vocabulary: null, AssistantContentMode.Ignore, TemporalValidityMode.Ignore, mode)), + }; + + foreach (var (rung, prompt) in prompts) + prompt.Should().Contain(expected, $"the {rung} rung must honour ExtractionProvenanceMode"); + } + + [Fact] + public void NoExtractorRungChangesItsPromptAtTheDefault() + { + // Prompt bytes are fingerprinted into every measured run, and the batch rung additionally uses + // its prompt for TOKEN ACCOUNTING -- an instruction it appends but does not count would make + // the frozen batch plan under-estimate by exactly that text. + var prompts = new[] + { + LlmFactExtractor.BuildSystemPrompt( + AssistantContentMode.Ignore, TemporalValidityMode.Ignore, ExtractionProvenanceMode.Batch), + LlmUnifiedMemoryExtractor.BuildSystemPrompt( + AssistantContentMode.Ignore, [], TemporalValidityMode.Ignore, ExtractionProvenanceMode.Batch), + LlmMultiSessionUnifiedMemoryExtractor.BuildSystemPrompt( + vocabulary: null, AssistantContentMode.Ignore, TemporalValidityMode.Ignore, + ExtractionProvenanceMode.Batch), + }; + + foreach (var prompt in prompts) + prompt.Should().NotContain("source_turn"); + } + + [Fact] + public void TheDefaultTranscriptIsUnnumbered() + { + // Prompt AND transcript bytes are fingerprinted into every measured run. Numbering by default + // would invalidate every sealed base without changing a single setting. + var messages = Messages(2); + + ConversationTextBuilder.Build(messages).Should().Be("user: turn 1\nuser: turn 2"); + } + + [Fact] + public void TheNumberedTranscriptIsOneBasedAndPositional() + { + // The numbering IS the contract the resolver indexes against; if these two ever disagreed, + // every fact would be attributed to the wrong turn and nothing would report an error. + ConversationTextBuilder.BuildNumbered(Messages(3)) + .Should().Be("[1] user: turn 1\n[2] user: turn 2\n[3] user: turn 3"); + } + + [Fact] + public void TheNumberedTranscriptHandlesAnEmptyConversation() + { + ConversationTextBuilder.BuildNumbered([]).Should().BeEmpty(); + } + + private static IReadOnlyList Messages(int count) => + Enumerable.Range(1, count).Select(index => new Message + { + MessageId = $"msg-{index}", + ConversationId = "c-1", + SessionId = "s-1", + Role = "user", + Content = $"turn {index}", + TimestampUtc = DateTimeOffset.UnixEpoch.AddMinutes(index), + }).ToArray(); +} diff --git a/tests/AgentMemory.Tests.Unit/Extraction/PerMessageTrustTests.cs b/tests/AgentMemory.Tests.Unit/Extraction/PerMessageTrustTests.cs new file mode 100644 index 00000000..cb89e11a --- /dev/null +++ b/tests/AgentMemory.Tests.Unit/Extraction/PerMessageTrustTests.cs @@ -0,0 +1,225 @@ +using FluentAssertions; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; +using AgentMemory.Abstractions.Domain; +using AgentMemory.Abstractions.Options; +using AgentMemory.Abstractions.Repositories; +using AgentMemory.Abstractions.Services; +using AgentMemory.Core.Extraction; +using AgentMemory.Core.Services; +using AgentMemory.Extraction.Llm; +using NSubstitute; +using Xunit; + +namespace AgentMemory.Tests.Unit.Extraction; + +/// +/// The falsifier for per-message trust: a batch containing both a user's claim and the model's own must +/// not record them identically. +/// +/// +/// +/// Trust was stamped once per extraction request and applied to every item in it. That is invisible +/// while AssistantContentMode.Ignore ships, because nothing assistant-derived is extracted at +/// all — and it becomes a defect the instant the mode is switched on, at which point every claim the +/// model made about the world is stored with the same label as the ones the user typed. +/// +/// +/// The failure is silent and permanent: nothing errors, the graph looks correct, and the +/// distinction the enum exists to draw is gone by the time anyone queries for it. So this asserts the +/// distribution, not just that a mapping function returns the right value — a correct mapping +/// that is never reached would satisfy the latter. +/// +/// +/// Our own NAMS subsystem has always mapped "assistant" => ModelGenerated correctly. One +/// subsystem getting it right while the other could not express it is what made this worth closing. +/// +/// +public sealed class PerMessageTrustTests +{ + private readonly IEmbeddingOrchestrator _orchestrator = Substitute.For(); + private readonly IEntityRepository _entityRepo = Substitute.For(); + private readonly IFactRepository _factRepo = Substitute.For(); + private readonly IPreferenceRepository _prefRepo = Substitute.For(); + private readonly IRelationshipRepository _relRepo = Substitute.For(); + private readonly IClock _clock = Substitute.For(); + private readonly IIdGenerator _idGen = Substitute.For(); + + private readonly List _writtenFacts = []; + private readonly List _writtenPreferences = []; + + public PerMessageTrustTests() + { + _clock.UtcNow.Returns(DateTimeOffset.UtcNow); + _idGen.GenerateId().Returns(_ => Guid.NewGuid().ToString("N")); + _orchestrator.EmbedAsync(Arg.Any(), Arg.Any()).Returns(new float[8]); + + _factRepo.UpsertAsync(Arg.Any(), Arg.Any()) + .Returns(ci => + { + _writtenFacts.Add(ci.Arg()); + return Task.FromResult(ci.Arg()); + }); + _prefRepo.UpsertAsync(Arg.Any(), Arg.Any()) + .Returns(ci => + { + _writtenPreferences.Add(ci.Arg()); + return Task.FromResult(ci.Arg()); + }); + } + + private PersistenceStage CreateSut() => + new(_orchestrator, _entityRepo, _factRepo, _prefRepo, _relRepo, _clock, _idGen, + NullLogger.Instance, + new PassThroughMemoryPersistenceTransaction(), Options.Create(new ExtractionOptions())); + + private static ExtractionStageResult WithFacts(params ExtractedFact[] facts) => + new() { FilteredFacts = facts }; + + private static ExtractedFact Fact(string @object, string? sourceRole) => new() + { + Subject = "user", + Predicate = "mentioned", + Object = @object, + Confidence = 0.9, + SourceRole = sourceRole, + }; + + // ── the distribution ────────────────────────────────────────────────── + + [Fact] + public async Task AMixedBatchRecordsTwoDifferentTrustLevels() + { + // THE falsifier. One request, two provenances. If this collapses to a single value the enum's + // central distinction is lost at the exact moment it first carries weight. + var extraction = WithFacts( + Fact("Zurich", sourceRole: "user"), + Fact("the 14:05 train", sourceRole: "assistant")); + + await CreateSut().PersistAsync(extraction, trustLevel: MemoryTrustLevel.UserProvided); + + _writtenFacts.Should().HaveCount(2); + _writtenFacts.Single(f => f.Object == "Zurich").Metadata.GetTrustLevel() + .Should().Be(MemoryTrustLevel.UserProvided); + _writtenFacts.Single(f => f.Object == "the 14:05 train").Metadata.GetTrustLevel() + .Should().Be(MemoryTrustLevel.ModelGenerated); + } + + [Fact] + public async Task AnAssistantSourcedPreferenceIsAlsoDistinguished() + { + // A preference the assistant attributed to the user becomes a durable statement about that + // user, and afterwards is indistinguishable from one they actually stated. + var extraction = new ExtractionStageResult + { + FilteredPreferences = + [ + new ExtractedPreference { Category = "style", PreferenceText = "dark mode", Confidence = 0.9, SourceRole = "user" }, + new ExtractedPreference { Category = "travel", PreferenceText = "aisle seats", Confidence = 0.9, SourceRole = "assistant" }, + ], + }; + + await CreateSut().PersistAsync(extraction, trustLevel: MemoryTrustLevel.UserProvided); + + _writtenPreferences.Single(p => p.PreferenceText == "dark mode").Metadata.GetTrustLevel() + .Should().Be(MemoryTrustLevel.UserProvided); + _writtenPreferences.Single(p => p.PreferenceText == "aisle seats").Metadata.GetTrustLevel() + .Should().Be(MemoryTrustLevel.ModelGenerated); + } + + // ── the unchanged direction ─────────────────────────────────────────── + + [Fact] + public async Task NoReportedRoleLeavesTheRequestTrustLevelExactlyAsItWas() + { + // The byte-identical guarantee for every host on shipped defaults: with AssistantContentMode + // .Ignore no extractor populates SourceRole, so every item takes this path and nothing moved. + var extraction = WithFacts(Fact("Zurich", sourceRole: null)); + + await CreateSut().PersistAsync(extraction, trustLevel: MemoryTrustLevel.VerifiedExternal); + + _writtenFacts.Should().ContainSingle() + .Which.Metadata.GetTrustLevel().Should().Be(MemoryTrustLevel.VerifiedExternal); + } + + [Theory] + [InlineData("user")] + [InlineData("system")] + [InlineData("tool")] + [InlineData("developer")] + [InlineData("")] + [InlineData("ASSISTANT_BOT")] + public async Task NoOtherRoleMovesTrustInAnyDirection(string role) + { + // Only "assistant" is interpreted, on purpose. MemoryTrustLevel is ordered so >= means "at + // least this trusted", and the default request trust is Untrusted -- so mapping "user" or + // "tool" would RAISE trust on hosts that never asked for any, on the strength of a label the + // model wrote about itself. Admission bypass and the system-role gate both compare with >=, + // which makes that a security-relevant direction rather than a cosmetic one. + var extraction = WithFacts(Fact("Zurich", sourceRole: role)); + + await CreateSut().PersistAsync(extraction); + + _writtenFacts.Should().ContainSingle() + .Which.Metadata.GetTrustLevel().Should().Be(MemoryTrustLevel.Untrusted); + } + + [Fact] + public async Task TheRoleMatchIsCaseInsensitive() + { + var extraction = WithFacts(Fact("the 14:05 train", sourceRole: "Assistant")); + + await CreateSut().PersistAsync(extraction, trustLevel: MemoryTrustLevel.UserProvided); + + _writtenFacts.Should().ContainSingle() + .Which.Metadata.GetTrustLevel().Should().Be(MemoryTrustLevel.ModelGenerated); + } + + [Fact] + public async Task AHostsOwnDeclarationIsNeverDemotedByAModelSelfReport() + { + // ApplicationTrusted (5) outranks ModelGenerated (2). A host that declared the whole ingestion + // trusted made a statement about the ingestion; refinement composes with that monotonic rule + // rather than competing with it, so this is max, not override. + var extraction = WithFacts(Fact("the 14:05 train", sourceRole: "assistant")); + + await CreateSut().PersistAsync(extraction, trustLevel: MemoryTrustLevel.ApplicationTrusted); + + _writtenFacts.Should().ContainSingle() + .Which.Metadata.GetTrustLevel().Should().Be(MemoryTrustLevel.ApplicationTrusted); + } + + // ── the prompt side ─────────────────────────────────────────────────── + + [Fact] + public void TheDefaultPromptNeverAsksForARole() + { + // Prompt bytes are fingerprinted into every measured run. At Ignore nothing assistant-derived + // is extracted, so the field would have one possible value and would buy nothing for the + // sealed bases it invalidated. + ExtractionPromptSemantics.AssistantContentInstruction(AssistantContentMode.Ignore) + .Should().BeEmpty(); + } + + [Theory] + [InlineData(AssistantContentMode.Utterance)] + [InlineData(AssistantContentMode.Fact)] + public void BothAssistantModesAskForTheRole(AssistantContentMode mode) + { + // A mode that extracts assistant content without asking which turn it came from reintroduces + // exactly the defect this closes, so neither mode may ship without the request. + ExtractionPromptSemantics.AssistantContentInstruction(mode) + .Should().Contain("source_role"); + } + + [Theory] + [InlineData(AssistantContentMode.Utterance)] + [InlineData(AssistantContentMode.Fact)] + public void TheRoleRequestDefaultsToUserWhenTheModelIsUnsure(AssistantContentMode mode) + { + // The instruction must name a default, and it must be the conservative one. Left open, an + // unsure model would pick freely -- and "assistant" is the value that RAISES trust here. + ExtractionPromptSemantics.AssistantContentInstruction(mode) + .Should().MatchRegex("(?i)unsure.*user"); + } +} diff --git a/tests/AgentMemory.Tests.Unit/Extraction/WriteTimeSupersessionTests.cs b/tests/AgentMemory.Tests.Unit/Extraction/WriteTimeSupersessionTests.cs new file mode 100644 index 00000000..9a61cf88 --- /dev/null +++ b/tests/AgentMemory.Tests.Unit/Extraction/WriteTimeSupersessionTests.cs @@ -0,0 +1,212 @@ +using FluentAssertions; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; +using AgentMemory.Abstractions.Domain; +using AgentMemory.Abstractions.Options; +using AgentMemory.Abstractions.Repositories; +using AgentMemory.Abstractions.Services; +using AgentMemory.Core.Extraction; +using AgentMemory.Core.Memory; +using AgentMemory.Core.Services; +using NSubstitute; +using Xunit; + +namespace AgentMemory.Tests.Unit.Extraction; + +/// +/// Write-time supersession (M1): a new assertion about a functional relation replaces the one it +/// contradicts, instead of accumulating beside it. +/// +/// +/// +/// The falsifier has two halves and both must hold, because each is trivially satisfiable +/// alone. Superseding everything would shrink the graph beautifully and destroy true facts; +/// superseding nothing keeps every fact and leaves the graph growing with the conversation rather than +/// with what is true. Fewer facts is only a win if nothing true was closed. +/// +/// +/// The dangerous direction is the second one, so it is the one covered hardest: a multi-valued +/// predicate must never be superseded, and "multi-valued" is the default for everything the vocabulary +/// has not explicitly declared functional — including every predicate the extractor invents. +/// +/// +public sealed class WriteTimeSupersessionTests +{ + private readonly IEmbeddingOrchestrator _orchestrator = Substitute.For(); + private readonly IEntityRepository _entityRepo = Substitute.For(); + private readonly IFactRepository _factRepo = Substitute.For(); + private readonly IPreferenceRepository _prefRepo = Substitute.For(); + private readonly IRelationshipRepository _relRepo = Substitute.For(); + private readonly IClock _clock = Substitute.For(); + private readonly IIdGenerator _idGen = Substitute.For(); + + private readonly List<(string Loser, string Winner)> _superseded = []; + + public WriteTimeSupersessionTests() + { + _clock.UtcNow.Returns(DateTimeOffset.UtcNow); + _idGen.GenerateId().Returns(_ => Guid.NewGuid().ToString("N")); + _orchestrator.EmbedAsync(Arg.Any(), Arg.Any()).Returns(new float[8]); + + _factRepo.UpsertAsync(Arg.Any(), Arg.Any()) + .Returns(ci => Task.FromResult(ci.Arg())); + _factRepo.FindSupersededCandidatesAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any(), Arg.Any()) + .Returns(Task.FromResult>([Stored("Basel")])); + _factRepo.SupersedeAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(ci => + { + _superseded.Add((ci.ArgAt(0), ci.ArgAt(1))); + return Task.FromResult(true); + }); + } + + private static Fact Stored(string @object) => new() + { + FactId = $"old-{@object}", + Subject = "user", + Predicate = "lives in", + Object = @object, + Confidence = 0.9, + CreatedAtUtc = DateTimeOffset.UnixEpoch, + }; + + private PersistenceStage CreateSut(bool supersede) => + new(_orchestrator, _entityRepo, _factRepo, _prefRepo, _relRepo, _clock, _idGen, + NullLogger.Instance, + new PassThroughMemoryPersistenceTransaction(), + Options.Create(new ExtractionOptions + { + SupersedeReplacedFacts = supersede, + // The batch path bypasses the per-item write hook; the item path is what this covers. + EnableBatchMemoryUpserts = false, + })); + + private static ExtractionStageResult Incoming(string predicate, string @object) => new() + { + FilteredFacts = + [ + new ExtractedFact + { + Subject = "user", Predicate = predicate, Object = @object, Confidence = 0.9, + }, + ], + }; + + // ── half one: the replacement happens ───────────────────────────────── + + [Fact] + public async Task ANewValueForAFunctionalRelationSupersedesTheOldOne() + { + await CreateSut(supersede: true).PersistAsync(Incoming("lives in", "Zurich"), ownerId: "alice"); + + _superseded.Should().ContainSingle().Which.Loser.Should().Be("old-Basel"); + } + + [Fact] + public async Task TheSupersessionIsOwnerScoped() + { + // Supersession closes a fact. A cross-owner one would close somebody else's, from a + // conversation they were not part of -- the worst shape this feature could take. + await CreateSut(supersede: true).PersistAsync(Incoming("lives in", "Zurich"), ownerId: "alice"); + + await _factRepo.Received().FindSupersededCandidatesAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Is(scope => scope != null && scope.OwnerId == "alice" && !scope.IncludeShared), + Arg.Any()); + } + + // ── half two: what must NOT be replaced ────────────────────────────── + + [Fact] + public async Task AMultiValuedRelationIsNeverSuperseded() + { + // THE dangerous direction. A person likes many things; closing "likes coffee" when "likes tea" + // arrives destroys a true fact and leaves a graph that still looks correct. The store is not + // even asked, so this costs no query either. + await CreateSut(supersede: true).PersistAsync(Incoming("likes", "tea"), ownerId: "alice"); + + _superseded.Should().BeEmpty(); + await _factRepo.DidNotReceive().FindSupersededCandidatesAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any(), Arg.Any()); + } + + [Theory] + [InlineData("attended")] // an event: additive by nature + [InlineData("owns")] // a state, but multi-valued + [InlineData("is interested in")] + [InlineData("vibed with")] // outside the vocabulary entirely + public async Task NothingUndeclaredIsEverSuperseded(string predicate) + { + await CreateSut(supersede: true).PersistAsync(Incoming(predicate, "something"), ownerId: "alice"); + + _superseded.Should().BeEmpty(); + } + + [Fact] + public async Task TheFeatureIsOffByDefault() + { + // It changes what live recall returns, and every recorded measurement was taken with + // append-only writes. A default flip would move results with no setting changed. + new ExtractionOptions().SupersedeReplacedFacts.Should().BeFalse(); + + await CreateSut(supersede: false).PersistAsync(Incoming("lives in", "Zurich"), ownerId: "alice"); + + _superseded.Should().BeEmpty(); + } + + [Fact] + public async Task AStoreFailureLeavesTheFactStoredRatherThanFailingTheIngestion() + { + // Losing a supersession costs precision in live recall; failing the ingestion loses the memory + // itself. The write already succeeded when this runs, so the fallback is exactly the + // append-only behaviour -- never a half-resolved graph. + _factRepo.FindSupersededCandidatesAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any(), Arg.Any()) + .Returns>>(_ => throw new InvalidOperationException("store is down")); + + var result = await CreateSut(supersede: true) + .PersistAsync(Incoming("lives in", "Zurich"), ownerId: "alice"); + + result.FactCount.Should().Be(1); + } + + // ── the cardinality declaration itself ──────────────────────────────── + + [Fact] + public void OnlyDeclaredRelationsAreFunctional() + { + MemoryRelationCardinality.IsSingleValued("lives in").Should().BeTrue(); + MemoryRelationCardinality.IsSingleValued("works at").Should().BeTrue(); + MemoryRelationCardinality.IsSingleValued("likes").Should().BeFalse(); + MemoryRelationCardinality.IsSingleValued("visited").Should().BeFalse(); + MemoryRelationCardinality.IsSingleValued("").Should().BeFalse(); + MemoryRelationCardinality.IsSingleValued(null).Should().BeFalse(); + } + + [Fact] + public void SurfaceFormsOfAFunctionalRelationAreFunctionalToo() + { + // Otherwise "lived in" would accumulate beside "lives in" and both stay live -- the exact + // accumulation this exists to stop, reappearing through a synonym. + MemoryRelationCardinality.IsSingleValued("LIVES IN").Should().BeTrue(); + MemoryRelationCardinality.IsSingleValued("lives_in").Should().BeTrue(); + } + + [Fact] + public void TheFunctionalSetIsSmallAndDeliberate() + { + // A guard on the direction of drift. Declaring a relation functional is a licence to close + // facts, so the set growing quietly -- especially past the state relations into events -- is + // the failure worth catching in review rather than in a corpus. + var functional = MemoryRelationCardinality.SingleValuedPredicates; + + functional.Should().HaveCountLessThan(12, + "each entry licenses closing a fact; a large set means someone stopped thinking about it"); + functional.Should().NotContain("likes").And.NotContain("owns").And.NotContain("visited"); + } +} diff --git a/tests/AgentMemory.Tests.Unit/Infrastructure/AbstractionsContractGuardTests.cs b/tests/AgentMemory.Tests.Unit/Infrastructure/AbstractionsContractGuardTests.cs index 6bd8e2b0..6104145c 100644 --- a/tests/AgentMemory.Tests.Unit/Infrastructure/AbstractionsContractGuardTests.cs +++ b/tests/AgentMemory.Tests.Unit/Infrastructure/AbstractionsContractGuardTests.cs @@ -19,7 +19,7 @@ public sealed class AbstractionsContractGuardTests private const int DocumentedServiceInterfaces = 41; // +IMultiSessionUnifiedMemoryExtractor (M-27-V2 LAB-B1) private const int DocumentedRepositoryInterfaces = 11; private const int DocumentedDomainRecords = 52; // +MemoryContextSectionDiagnostics (PLAN 4.1) - private const int DocumentedEnums = 27; // +ValidTimeMode (PLAN 1.1) + private const int DocumentedEnums = 29; // +TraceKind (7.1), +ExtractionProvenanceMode (9.3) private static IEnumerable PublicTypes() => Abstractions.GetTypes().Where(t => t.IsPublic); diff --git a/tests/AgentMemory.Tests.Unit/Infrastructure/Neo4jTransactionRunnerTests.cs b/tests/AgentMemory.Tests.Unit/Infrastructure/Neo4jTransactionRunnerTests.cs index 31023185..40c2db81 100644 --- a/tests/AgentMemory.Tests.Unit/Infrastructure/Neo4jTransactionRunnerTests.cs +++ b/tests/AgentMemory.Tests.Unit/Infrastructure/Neo4jTransactionRunnerTests.cs @@ -142,8 +142,19 @@ public async Task ReadAsync_QuerySpansCarryKnownOrUnknownFingerprintWithoutCyphe using var listener = new ActivityListener { ShouldListenTo = source => source.Name == AgentMemoryDiagnostics.SourceName, - Sample = (ref ActivityCreationOptions _) => - ActivitySamplingResult.AllDataAndRecorded, + // Scoped to the two db spans, not the whole source. ActivityListener is process-global and + // sampling is a UNION across listeners, so sampling everything here forces creation of + // every AgentMemory span in every test class running concurrently -- which is exactly what + // breaks a neighbour's "nothing is measured when no listener wants the data" assertion. + // Measured: that neighbour failed roughly 1 run in 4 until this was scoped. + // + // BOTH names are needed, not just the one asserted on: the query-span wrapper is only + // installed when the enclosing memory.db.tx activity exists, so declining the parent + // silently yields zero query spans rather than the two this test is about. + Sample = (ref ActivityCreationOptions options) => + options.Name is "memory.db.query" or "memory.db.tx" + ? ActivitySamplingResult.AllDataAndRecorded + : ActivitySamplingResult.None, ActivityStopped = activity => { if (activity.OperationName == "memory.db.query") @@ -189,8 +200,15 @@ public async Task ReadAsync_TransactionSpanReportsLabelledEntryDelayEstimate() using var listener = new ActivityListener { ShouldListenTo = source => source.Name == AgentMemoryDiagnostics.SourceName, - Sample = (ref ActivityCreationOptions _) => - ActivitySamplingResult.AllDataAndRecorded, + // Scoped to this one span, not the whole source. ActivityListener is process-global and + // sampling is a UNION across listeners, so sampling everything here forces creation of + // every AgentMemory span in every test class running concurrently -- which is exactly what + // breaks a neighbour's "nothing is measured when no listener wants the data" assertion. + // Measured: that neighbour failed roughly 1 run in 4 until this was scoped. + Sample = (ref ActivityCreationOptions options) => + options.Name == "memory.db.tx" + ? ActivitySamplingResult.AllDataAndRecorded + : ActivitySamplingResult.None, ActivityStopped = activity => { if (activity.OperationName == "memory.db.tx") diff --git a/tests/AgentMemory.Tests.Unit/Infrastructure/SchemaBootstrapperTests.cs b/tests/AgentMemory.Tests.Unit/Infrastructure/SchemaBootstrapperTests.cs index 40b86453..2c7e8a11 100644 --- a/tests/AgentMemory.Tests.Unit/Infrastructure/SchemaBootstrapperTests.cs +++ b/tests/AgentMemory.Tests.Unit/Infrastructure/SchemaBootstrapperTests.cs @@ -1,4 +1,4 @@ -using FluentAssertions; +using FluentAssertions; using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Options; using AgentMemory.Abstractions.Exceptions; @@ -80,8 +80,9 @@ public async Task BootstrapAsync_ExecutesExpectedTotalNumberOfStatements() // 12 constraints + 3 fulltext + 6 vector + 26 property = 47 // +1 fact_merge_key_idx (L11); +1 memory_read_audit_memory_id_idx (BUG-A2); - // +1 message_session_timestamp_idx; +1 fact_predicate_key_idx (unindexed hot predicates). - executedStatements.Should().HaveCount(49); + // +1 message_session_timestamp_idx; +1 fact_predicate_key_idx (unindexed hot predicates); + // +1 trace_kind_idx (PLAN 7.2, the procedure promotion marker). + executedStatements.Should().HaveCount(50); } [Fact] @@ -216,7 +217,7 @@ public async Task BootstrapAsync_ExecutesAllPropertyIndexes() var propertyIndexes = executedStatements .Where(s => s.StartsWith("CREATE INDEX") || s.StartsWith("CREATE POINT INDEX")) .ToList(); - propertyIndexes.Should().HaveCount(28); + propertyIndexes.Should().HaveCount(29); propertyIndexes.Should().Contain(s => s.Contains("conversation_session_idx")); propertyIndexes.Should().Contain(s => s.Contains("conversation_archived_idx")); propertyIndexes.Should().Contain(s => s.Contains("message_timestamp")); @@ -228,6 +229,9 @@ public async Task BootstrapAsync_ExecutesAllPropertyIndexes() propertyIndexes.Should().Contain(s => s.Contains("preference_category")); propertyIndexes.Should().Contain(s => s.Contains("trace_session_idx")); propertyIndexes.Should().Contain(s => s.Contains("trace_success_idx")); + // The procedure promotion marker. Asserted by NAME as well as by count, so a future edit + // cannot swap one index for another and keep the total looking right. + propertyIndexes.Should().Contain(s => s.Contains("trace_kind_idx")); propertyIndexes.Should().Contain(s => s.Contains("reasoning_step_timestamp")); propertyIndexes.Should().Contain(s => s.Contains("tool_call_status")); propertyIndexes.Should().Contain(s => s.Contains("schema_name_idx")); diff --git a/tests/AgentMemory.Tests.Unit/McpServer/McpHostOptionsTests.cs b/tests/AgentMemory.Tests.Unit/McpServer/McpHostOptionsTests.cs new file mode 100644 index 00000000..587e77f9 --- /dev/null +++ b/tests/AgentMemory.Tests.Unit/McpServer/McpHostOptionsTests.cs @@ -0,0 +1,160 @@ +using AgentMemory.McpHost; +using FluentAssertions; +using Microsoft.Extensions.Logging; +using Xunit; + +namespace AgentMemory.Tests.Unit.McpServer; + +/// +/// The turnkey host's configuration: defaults, precedence, and the refusals. +/// +/// +/// +/// Option parsing is the part of a host most likely to be quietly wrong and least likely to be caught +/// by running it — a server that starts and answers looks identical whether it read the flag or +/// ignored it. So parsing is separated from running and tested without a database, a provider or a +/// port. +/// +/// +/// The safety-relevant case is --read-only. A typo that is silently ignored produces a fully +/// writable server the operator believes is read-only, which is the one failure this host must not +/// have. +/// +/// +public sealed class McpHostOptionsTests +{ + /// Only the three genuinely required variables, so a test states its own inputs. + private static Func Env(params (string Key, string Value)[] extra) + { + var values = new Dictionary(StringComparer.Ordinal) + { + ["AZURE_OPENAI_ENDPOINT"] = "https://example.openai.azure.com/", + ["AZURE_OPENAI_API_KEY"] = "key", + ["NEO4J_PASSWORD"] = "password", + }; + foreach (var (key, value) in extra) values[key] = value; + return name => values.TryGetValue(name, out var value) ? value : null; + } + + private static McpHostOptions Parse(string[] args, Func environment) => + McpHostOptions.Parse(args, environment); + + // ── defaults ────────────────────────────────────────────────────────── + + [Fact] + public void TheDefaultsAreStdioReadWriteAndBootstrapping() + { + var options = Parse([], Env()); + + options.Transport.Should().Be(McpHostTransport.Stdio); + options.ReadOnly.Should().BeFalse(); + options.EnableGraphQuery.Should().BeFalse("arbitrary Cypher must be opt-in"); + options.Bootstrap.Should().BeTrue( + "a missing vector index returns no rows rather than an error, so a server started without " + + "the schema looks healthy and answers nothing"); + options.LogLevel.Should().Be(LogLevel.Information); + options.Neo4jUri.Should().Be("bolt://localhost:7687"); + options.Neo4jDatabase.Should().Be("neo4j"); + } + + // ── the refusals ────────────────────────────────────────────────────── + + [Fact] + public void AnUnknownFlagIsRejectedRatherThanIgnored() + { + // THE safety case. A silently-ignored "--read-onlyy" starts a fully writable server that the + // operator believes is read-only. + var act = () => Parse(["--read-onlyy"], Env()); + + act.Should().Throw().WithMessage("*--read-onlyy*"); + } + + [Theory] + [InlineData("AZURE_OPENAI_ENDPOINT")] + [InlineData("AZURE_OPENAI_API_KEY")] + [InlineData("NEO4J_PASSWORD")] + public void EachRequiredVariableIsRequiredByName(string variable) + { + // Named individually so the message says which one, and NEO4J_PASSWORD is required at all -- + // a blank password becomes an authentication failure at the first query, which from an MCP + // client is indistinguishable from an empty database. + var without = Env(); + Func missing = name => name == variable ? null : without(name); + + var act = () => Parse([], missing); + + act.Should().Throw().WithMessage($"*{variable}*"); + } + + [Fact] + public void AnUnknownTransportIsRejected() + { + var act = () => Parse(["--transport", "grpc"], Env()); + + act.Should().Throw().WithMessage("*stdio, http*"); + } + + [Fact] + public void AFlagWithoutItsValueIsRejected() + { + var act = () => Parse(["--transport"], Env()); + + act.Should().Throw().WithMessage("*requires a value*"); + } + + // ── precedence ──────────────────────────────────────────────────────── + + [Fact] + public void AFlagOverridesTheEnvironmentForOrdinarySettings() + { + var options = Parse( + ["--transport", "http"], Env(("AGENT_MEMORY_MCP_TRANSPORT", "stdio"))); + + options.Transport.Should().Be(McpHostTransport.Http); + } + + [Fact] + public void ReadOnlyIsTheUnionOfFlagAndEnvironmentRatherThanAnOverride() + { + // For a safety switch the union is the correct combination: someone who set the variable and + // someone who passed the flag both asked for read-only, and neither absence cancels the other. + Parse(["--read-only"], Env()).ReadOnly.Should().BeTrue(); + Parse([], Env(("AGENT_MEMORY_MCP_READ_ONLY", "true"))).ReadOnly.Should().BeTrue(); + Parse(["--read-only"], Env(("AGENT_MEMORY_MCP_READ_ONLY", "false"))).ReadOnly + .Should().BeTrue("a flag asking for read-only cannot be cancelled by a variable saying otherwise"); + } + + [Theory] + [InlineData("1")] + [InlineData("true")] + [InlineData("TRUE")] + [InlineData("yes")] + public void TheUsualAffirmativesAllEnableABooleanVariable(string value) => + Parse([], Env(("AGENT_MEMORY_MCP_READ_ONLY", value))).ReadOnly.Should().BeTrue(); + + [Theory] + [InlineData("0")] + [InlineData("false")] + [InlineData("")] + [InlineData("maybe")] + public void AnythingElseLeavesItOff(string value) => + Parse([], Env(("AGENT_MEMORY_MCP_READ_ONLY", value))).ReadOnly.Should().BeFalse(); + + [Fact] + public void ABlankEnvironmentVariableIsTreatedAsAbsent() + { + // An exported-but-empty variable is how a container ends up binding to "" and failing at + // startup with a message about a URL nobody wrote. + var options = Parse([], Env(("AGENT_MEMORY_MCP_URL", " "), ("NEO4J_URI", ""))); + + options.HttpUrl.Should().Be("http://localhost:5233"); + options.Neo4jUri.Should().Be("bolt://localhost:7687"); + } + + [Fact] + public void BootstrapIsDisabledByEitherTheFlagOrTheVariable() + { + Parse(["--no-bootstrap"], Env()).Bootstrap.Should().BeFalse(); + Parse([], Env(("AGENT_MEMORY_MCP_NO_BOOTSTRAP", "1"))).Bootstrap.Should().BeFalse(); + } +} diff --git a/tests/AgentMemory.Tests.Unit/McpServer/ReadOnlyModeTests.cs b/tests/AgentMemory.Tests.Unit/McpServer/ReadOnlyModeTests.cs new file mode 100644 index 00000000..277bad01 --- /dev/null +++ b/tests/AgentMemory.Tests.Unit/McpServer/ReadOnlyModeTests.cs @@ -0,0 +1,187 @@ +using System.Reflection; +using AgentMemory.McpServer; +using FluentAssertions; +using Microsoft.Extensions.DependencyInjection; +using ModelContextProtocol.Server; +using Xunit; + +namespace AgentMemory.Tests.Unit.McpServer; + +/// +/// --read-only must be a fact about the server, not a promise about it. +/// +/// +/// +/// A read-only server that still exposes one write tool is worse than one with no such mode: +/// the operator believes the guarantee and stops checking. The failure is also silent by nature — +/// nothing errors until a model calls the tool and the store changes. +/// +/// +/// So the guard here is not "the current list is right" — a list can be re-read — but that no tool +/// can escape classification. A tool added next year, with nobody thinking about read-only mode, +/// must fail this test rather than quietly ship as callable. +/// +/// +public sealed class ReadOnlyModeTests +{ + /// Every tool name the server actually declares, read from the attributes. + /// + /// Reflected rather than listed, because a hand-maintained inventory is precisely the thing that + /// drifts. Reading the attributes means this test sees what the server exposes, not what someone + /// remembered to write down. + /// + private static IReadOnlyList DeclaredToolNames() + { + var names = typeof(AgentMemoryMcpOptions).Assembly + .GetTypes() + .Where(type => type.GetCustomAttribute() is not null) + .SelectMany(type => type.GetMethods(BindingFlags.Public | BindingFlags.Static | BindingFlags.Instance)) + .Select(method => method.GetCustomAttribute()?.Name) + .Where(name => !string.IsNullOrEmpty(name)) + .Select(name => name!) + .Distinct(StringComparer.Ordinal) + .ToList(); + + names.Should().NotBeEmpty("the reflection above must actually find the tools, or this whole " + + "test class passes vacuously and guards nothing"); + return names; + } + + [Fact] + public void EveryDeclaredToolIsClassifiedAsExactlyOneOfReadOrWrite() + { + // THE guard. A tool in neither set is a tool nobody classified; a tool in both is a + // contradiction. Either way the read-only guarantee stops meaning anything, and neither shows + // up at runtime. + foreach (var name in DeclaredToolNames()) + { + var isRead = McpToolAccess.ReadTools.Contains(name); + var isWrite = McpToolAccess.WriteTools.Contains(name); + + (isRead ^ isWrite).Should().BeTrue( + $"'{name}' must be classified as exactly one of read or write in McpToolAccess " + + "(read={0}, write={1}); an unclassified tool silently decides whether --read-only " + + "is true", isRead, isWrite); + } + } + + [Fact] + public void TheClassificationNamesNoToolThatDoesNotExist() + { + // The other drift direction: a renamed or removed tool leaving a stale entry behind. Harmless + // on its own, but it makes the lists untrustworthy, and these lists are the only statement of + // what a read-only server can do. + var declared = DeclaredToolNames().ToHashSet(StringComparer.Ordinal); + + McpToolAccess.ReadTools.Concat(McpToolAccess.WriteTools) + .Should().OnlyContain(name => declared.Contains(name)); + } + + [Fact] + public void AnUnknownToolIsTreatedAsAWrite() + { + // Fail closed. A tool this classification has never heard of is one nobody has reviewed, and + // the safe reading of "unreviewed" is "assume it changes something". + McpToolAccess.IsReadOnly("memory_do_something_new").Should().BeFalse(); + McpToolAccess.IsReadOnly(null).Should().BeFalse(); + McpToolAccess.IsReadOnly("").Should().BeFalse(); + } + + [Theory] + [InlineData("memory_store_message")] + [InlineData("memory_add_fact")] + [InlineData("memory_invalidate")] + [InlineData("memory_supersede")] + [InlineData("extract_and_persist")] + [InlineData("memory_generate_embeddings")] + public void TheObviousWritesAreWrites(string name) => + McpToolAccess.IsReadOnly(name).Should().BeFalse(); + + [Fact] + public void GraphQueryIsWithheldFromReadOnlyServersDespiteBeingNominallyARead() + { + // It takes arbitrary Cypher. It is gated behind EnableGraphQuery and validated read-only at + // execution, but a mode whose entire value is "this cannot change anything" must not rest on a + // second component's parser. + McpToolAccess.IsReadOnly("graph_query").Should().BeFalse(); + } + + [Theory] + [InlineData("memory_search")] + [InlineData("memory_get_context")] + [InlineData("memory_get_conversation")] + [InlineData("memory_list_sessions")] + [InlineData("memory_get_entity")] + [InlineData("memory_get_observations")] + public void TheReadsSurviveReadOnlyMode(string name) + { + // The other half: a read-only server that exposes nothing useful is a mode nobody will turn + // on, and the point is to make the safe configuration the attractive one. + McpToolAccess.IsReadOnly(name).Should().BeTrue(); + } + + [Fact] + public void ReadOnlyIsOffByDefault() + { + new AgentMemoryMcpOptions().ReadOnly.Should().BeFalse(); + } + + // ── the filter, on a real registration ─────────────────────────────── + + private static IReadOnlyList RegisteredToolNames(bool readOnly) + { + var services = new ServiceCollection(); + services.AddMcpServer().AddAgentMemoryMcpTools(options => + { + options.ReadOnly = readOnly; + options.EnableGraphQuery = true; + }); + + // The SDK registers tools as singleton factories rather than instances, so the name is only + // readable by invoking one. Reading ImplementationInstance instead returned null for every + // descriptor -- which made the first version of the production filter a silent no-op that this + // test caught. + using var empty = new ServiceCollection().BuildServiceProvider(); + return services + .Where(descriptor => descriptor.ServiceType == typeof(McpServerTool)) + .Select(descriptor => (descriptor.ImplementationFactory?.Invoke(empty) as McpServerTool) + ?.ProtocolTool.Name) + .Where(name => name is not null) + .Select(name => name!) + .ToList(); + } + + [Fact] + public void ReadOnlyRegistrationExposesTheReadsAndNothingElse() + { + // Asserted on the registration rather than on the classification lists: the lists could be + // perfect while the filter never ran, and the server would look exactly the same until a model + // called a write tool. + var exposed = RegisteredToolNames(readOnly: true); + + exposed.Should().NotBeEmpty("a read-only server that exposes nothing is a mode nobody turns on"); + exposed.Should().OnlyContain(name => McpToolAccess.ReadTools.Contains(name)); + exposed.Should().NotContain("graph_query", + "it is withheld even with EnableGraphQuery set, because a mode guaranteeing 'nothing " + + "changes' must not rest on a query parser"); + } + + [Fact] + public void TheDefaultRegistrationIsUnchangedAndExposesEverything() + { + // The byte-identical guarantee for every existing host: read-only is opt-in, and turning it + // off must leave the server exactly as it was. + var exposed = RegisteredToolNames(readOnly: false); + + exposed.Should().BeEquivalentTo(DeclaredToolNames()); + } + + [Fact] + public void MostToolsAreWritesWhichIsWhyEnumeratingWritesIsTheSaferChoiceToDocument() + { + // Recorded as a fact rather than an opinion: the write set is the larger one, so "list the + // reads and treat the rest as writes" would have been the shorter list. It is still the wrong + // one, because forgetting to add to it exposes a write instead of hiding a read. + McpToolAccess.WriteTools.Count.Should().BeGreaterThan(McpToolAccess.ReadTools.Count); + } +} diff --git a/tests/AgentMemory.Tests.Unit/Queries/CypherQuerySnapshot.snap b/tests/AgentMemory.Tests.Unit/Queries/CypherQuerySnapshot.snap index d01249e6..bf3fc27f 100644 --- a/tests/AgentMemory.Tests.Unit/Queries/CypherQuerySnapshot.snap +++ b/tests/AgentMemory.Tests.Unit/Queries/CypherQuerySnapshot.snap @@ -1,4 +1,4 @@ -# Cypher Query Snapshot — 157 queries +# Cypher Query Snapshot — 158 queries # Auto-generated by CypherQuerySnapshotTests. # To regenerate: set UPDATE_CYPHER_SNAPSHOTS=1 and re-run the test. @@ -693,7 +693,8 @@ CREATE (t:ReasoningTrace { task: $task, outcome: $outcome, success: $success, - metadata: $metadata + metadata: $metadata, + trace_kind: $traceKind }) SET t.started_at = datetime($startedAt), t.completed_at = CASE WHEN $completedAt IS NOT NULL THEN datetime($completedAt) ELSE null END @@ -990,7 +991,7 @@ SHOW CONSTRAINTS YIELD name RETURN name SHOW INDEXES YIELD name RETURN name ## SchemaQueries.ShowIndexStates -SHOW INDEXES YIELD name, state, type RETURN name AS name, state AS state, type AS type +SHOW INDEXES YIELD name, state, type, populationPercent RETURN name AS name, state AS state, type AS type, populationPercent AS populationPercent ## SchemaQueries.ShowVectorIndexDimensions SHOW VECTOR INDEXES YIELD name, options RETURN name AS name, options['indexConfig']['vector.dimensions'] AS dimensions @@ -1004,6 +1005,9 @@ CREATE INDEX tool_call_status_idx IF NOT EXISTS FOR (tc:ToolCall) ON (tc.status) ## SchemaQueries.ToolNameConstraint CREATE CONSTRAINT tool_name IF NOT EXISTS FOR (t:Tool) REQUIRE t.name IS UNIQUE +## SchemaQueries.TraceKindIndex +CREATE INDEX trace_kind_idx IF NOT EXISTS FOR (t:ReasoningTrace) ON (t.trace_kind) + ## SchemaQueries.TraceOwnerIndex CREATE INDEX trace_owner_idx IF NOT EXISTS FOR (t:ReasoningTrace) ON (t.owner_id) diff --git a/tests/AgentMemory.Tests.Unit/Queries/CypherQuerySnapshotTests.cs b/tests/AgentMemory.Tests.Unit/Queries/CypherQuerySnapshotTests.cs index 10a4f66e..776f5e05 100644 --- a/tests/AgentMemory.Tests.Unit/Queries/CypherQuerySnapshotTests.cs +++ b/tests/AgentMemory.Tests.Unit/Queries/CypherQuerySnapshotTests.cs @@ -1,4 +1,4 @@ -using System.Runtime.CompilerServices; +using System.Runtime.CompilerServices; using System.Text; using System.Text.RegularExpressions; using FluentAssertions; @@ -37,7 +37,7 @@ private static string ResolveSnapshotPath([CallerFilePath] string? sourceFile = // ── Expected query inventory count ──────────────────────────────────────── // Update this constant whenever queries are deliberately added or removed. - private const int ExpectedQueryCount = 154; // +SchemaQueries.FactOwnerKeyIndex (measured: FindDuplicate runs on every fact write and had NO index entry point, planning a full 20,000-row scan; this seeks 100). // +SchemaQueries.MessageSessionIndex (measured: Neo4j will not seek a composite from a leading-column predicate alone, so 0007's (session_id, timestamp) gave GetRecentBySession nothing). // +SchemaQueries.MessageSessionTimestampIndex (Message.session_id is the predicate of the PRIMARY short-term recall path -- MessageQueries.cs:201, run on essentially every turn -- and nothing indexed it, so the plan was proportional to every message in the store rather than to the session; composite because TemporalQueries.cs:99-103 adds a trailing timestamp range to the same equality). // +SchemaQueries.FactPredicateKeyIndex (the same shape one column over: predicate_key sits at column 3 of fact_merge_key_idx and Neo4j serves a composite only on a matching PREFIX, so FactQueries.cs:88 had no index entry point at all -- a full :Fact scan across all owners whenever relation-completeness retrieval fires). // +SchemaQueries.MemoryReadAuditMemoryIdIndex (BUG-A2: HistoryQueries OPTIONAL MATCHes MemoryReadAudit on memory_id, which nothing indexed, so every history row scanned the whole label -- and the label grows ~25 rows per recall, so it degrades with TIME rather than data size). // +SchemaQueries.FactMergeKeyIndex (L11: every fact MERGE was an all-:Fact label scan; the composite {subject_key, object_key, predicate_key, owner_key} index is what makes the range-index key cap real for facts, which is why IndexKeyBudget.EnsureCompositeIndexable lands with it). // -1: SearchByCanonicalPredicates became an owner-conditional *method* (excluded, like GetBySubject) when the audit found it ignored IncludeShared and coerced a null-owner scope to the shared bucket. // +FactQueries.SelectFactsMissingCanonicalKeys/ApplyCanonicalKeys (Phase 1.1: canonical identity needs a C#-driven backfill, since Cypher's toLower diverges from ToLowerInvariant on U+0130). // +FactQueries.SearchByCanonicalPredicates (G3B.13: top-K is a relevance cutoff and cannot answer "how many", so a relation is retrieved whole via predicate_key). // +SchemaQueries.ShowIndexStates (BUG-S1: only vector-index dimensions were validated at bootstrap, so a FAILED range index degraded silently into full scans). base + ConsolidationQueries/SchemaQueries/TOUCHED/ConflictQueries consts. +MessageQueries.GetAllBySession (cycle-3); +SchemaQueries.ShowConstraintNames/ShowIndexNames (schema-check CLI); +SchemaPersistenceQueries.Save/DeactivateByName/LoadActiveByName/LoadByNameVersion/List/Exists/DeleteById (G4 schema-node CRUD, 7); +MemoryReadAudit constraint/index. Owner-conditional queries are *methods* (excluded): EntityQueries.ApplyConfidenceDelta/Delete/MergeEntities/SearchByLocation/SearchInBoundingBox/GetByType/FindSimilarByEmbedding, FactQueries.Delete/FindByTriple, PreferenceQueries.Delete, DecayQueries.PruneEntities/PruneFacts/PrunePreferences, ExtractorQueries.GetEntityProvenance, ReasoningQueries.ListTracesBySession (R2 owner-scoped 2026-06-13), ReasoningQueries.DeleteBySession (R6-C owner-scoped 2026-06-20: const→method, −1); +PreferenceQueries.UpsertBatch/RelationshipQueries.UpsertBatch (feat-04, +2). + private const int ExpectedQueryCount = 155; // +SchemaQueries.TraceKindIndex (PLAN 7.2: the promotion marker separating an episode from a reusable procedure -- seekable so a procedures-only search is a seek rather than a post-filter over the whole label, and mirroring trace_success_idx). // +SchemaQueries.FactOwnerKeyIndex (measured: FindDuplicate runs on every fact write and had NO index entry point, planning a full 20,000-row scan; this seeks 100). // +SchemaQueries.MessageSessionIndex (measured: Neo4j will not seek a composite from a leading-column predicate alone, so 0007's (session_id, timestamp) gave GetRecentBySession nothing). // +SchemaQueries.MessageSessionTimestampIndex (Message.session_id is the predicate of the PRIMARY short-term recall path -- MessageQueries.cs:201, run on essentially every turn -- and nothing indexed it, so the plan was proportional to every message in the store rather than to the session; composite because TemporalQueries.cs:99-103 adds a trailing timestamp range to the same equality). // +SchemaQueries.FactPredicateKeyIndex (the same shape one column over: predicate_key sits at column 3 of fact_merge_key_idx and Neo4j serves a composite only on a matching PREFIX, so FactQueries.cs:88 had no index entry point at all -- a full :Fact scan across all owners whenever relation-completeness retrieval fires). // +SchemaQueries.MemoryReadAuditMemoryIdIndex (BUG-A2: HistoryQueries OPTIONAL MATCHes MemoryReadAudit on memory_id, which nothing indexed, so every history row scanned the whole label -- and the label grows ~25 rows per recall, so it degrades with TIME rather than data size). // +SchemaQueries.FactMergeKeyIndex (L11: every fact MERGE was an all-:Fact label scan; the composite {subject_key, object_key, predicate_key, owner_key} index is what makes the range-index key cap real for facts, which is why IndexKeyBudget.EnsureCompositeIndexable lands with it). // -1: SearchByCanonicalPredicates became an owner-conditional *method* (excluded, like GetBySubject) when the audit found it ignored IncludeShared and coerced a null-owner scope to the shared bucket. // +FactQueries.SelectFactsMissingCanonicalKeys/ApplyCanonicalKeys (Phase 1.1: canonical identity needs a C#-driven backfill, since Cypher's toLower diverges from ToLowerInvariant on U+0130). // +FactQueries.SearchByCanonicalPredicates (G3B.13: top-K is a relevance cutoff and cannot answer "how many", so a relation is retrieved whole via predicate_key). // +SchemaQueries.ShowIndexStates (BUG-S1: only vector-index dimensions were validated at bootstrap, so a FAILED range index degraded silently into full scans). base + ConsolidationQueries/SchemaQueries/TOUCHED/ConflictQueries consts. +MessageQueries.GetAllBySession (cycle-3); +SchemaQueries.ShowConstraintNames/ShowIndexNames (schema-check CLI); +SchemaPersistenceQueries.Save/DeactivateByName/LoadActiveByName/LoadByNameVersion/List/Exists/DeleteById (G4 schema-node CRUD, 7); +MemoryReadAudit constraint/index. Owner-conditional queries are *methods* (excluded): EntityQueries.ApplyConfidenceDelta/Delete/MergeEntities/SearchByLocation/SearchInBoundingBox/GetByType/FindSimilarByEmbedding, FactQueries.Delete/FindByTriple, PreferenceQueries.Delete, DecayQueries.PruneEntities/PruneFacts/PrunePreferences, ExtractorQueries.GetEntityProvenance, ReasoningQueries.ListTracesBySession (R2 owner-scoped 2026-06-13), ReasoningQueries.DeleteBySession (R6-C owner-scoped 2026-06-20: const→method, −1); +PreferenceQueries.UpsertBatch/RelationshipQueries.UpsertBatch (feat-04, +2). // ── MemberData source ───────────────────────────────────────────────────── diff --git a/tests/AgentMemory.Tests.Unit/Queries/ProcedureTraceQueryTests.cs b/tests/AgentMemory.Tests.Unit/Queries/ProcedureTraceQueryTests.cs new file mode 100644 index 00000000..d1b74d80 --- /dev/null +++ b/tests/AgentMemory.Tests.Unit/Queries/ProcedureTraceQueryTests.cs @@ -0,0 +1,98 @@ +using AgentMemory.Neo4j.Queries; +using FluentAssertions; +using Xunit; + +namespace AgentMemory.Tests.Unit.Queries; + +/// +/// Promotion of a trace to a reusable procedure: the filter, and the exemption that makes it real. +/// +/// +/// A trace and a procedure are the same record read two ways — an episode says what happened once, +/// a procedure says what to do next time, and they differ by retrieval key. Without the prune +/// exemption the distinction cannot survive: retention orders by age alone and would delete a +/// promoted procedure as soon as newer traces arrived. +/// +public sealed class ProcedureTraceQueryTests +{ + [Fact] + public void TheSearchIsUnchangedWhenNoProcedureFilterIsRequested() + { + // The TCK guard. /get_similar_traces takes every default, so a non-null default here would + // change the Cypher it emits and break Gold 18/18 -- by filtering a corpus that holds no + // promoted traces at all, i.e. to zero. + var cypher = ReasoningQueries.SearchByTaskVector(false, true, true, 60); + + cypher.Should().NotContain("trace_kind"); + } + + [Fact] + public void ProceduresOnlySelectsPromotedTraces() + { + var cypher = ReasoningQueries.SearchByTaskVector(false, true, true, 60, proceduresOnly: true); + + cypher.Should().Contain("coalesce(node.trace_kind, 'episode') = 'procedure'"); + } + + [Fact] + public void ProceduresExcludedSelectsOrdinaryEpisodes() + { + var cypher = ReasoningQueries.SearchByTaskVector(false, true, true, 60, proceduresOnly: false); + + cypher.Should().Contain("coalesce(node.trace_kind, 'episode') <> 'procedure'"); + } + + [Fact] + public void TheProcedureFilterIsNullSafeForTracesWrittenBeforeItExisted() + { + // A trace stored before trace_kind has the property NULL. A NULL-unsafe comparison would make + // every legacy trace invisible to the episode filter -- silently emptying a corpus that is + // entirely legacy, which is exactly what a pre-existing store is. + foreach (var cypher in new[] + { + ReasoningQueries.SearchByTaskVector(false, false, true, 60, proceduresOnly: true), + ReasoningQueries.SearchByTaskVector(false, false, true, 60, proceduresOnly: false), + }) + { + cypher.Should().Contain("coalesce(node.trace_kind, 'episode')"); + } + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public void ThePruneExemptsPromotedProcedures(bool ownerIsShared) + { + // THE load-bearing clause. Retention orders by started_at with age as its only criterion and + // fires on every trace creation once a cap is set, so without this a promoted procedure is + // undone by recency and the capability does not exist. + var cypher = ReasoningQueries.PruneSessionTraces(ownerIsShared); + + cypher.Should().Contain("coalesce(t.trace_kind, 'episode') <> 'procedure'"); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public void ThePruneStillConfinesToASingleOwnerBucket(bool ownerIsShared) + { + // The exemption must not weaken the isolation guarantee it sits next to: a destructive write + // keyed by a guessable session_id must never collapse to "all owners". + var cypher = ReasoningQueries.PruneSessionTraces(ownerIsShared); + + if (ownerIsShared) cypher.Should().Contain("t.owner_id IS NULL"); + else cypher.Should().Contain("t.owner_id = $ownerId"); + } + + [Fact] + public void ThePruneExemptionIsNullSafeSoLegacyTracesAreStillPruned() + { + // Written as "is NOT a procedure" rather than "is an episode": a NULL-unsafe form would exempt + // every trace written before trace_kind existed, quietly turning a bounded store into an + // unbounded one -- a retention cap that silently stops capping. + var cypher = ReasoningQueries.PruneSessionTraces(false); + + cypher.Should().Contain("coalesce(t.trace_kind, 'episode')"); + cypher.Should().NotContain("t.trace_kind = 'episode'"); + } +} diff --git a/tools/AgentMemory.Cli/Commands/MemoryCommands.cs b/tools/AgentMemory.Cli/Commands/MemoryCommands.cs index e5e86d9b..2308d54a 100644 --- a/tools/AgentMemory.Cli/Commands/MemoryCommands.cs +++ b/tools/AgentMemory.Cli/Commands/MemoryCommands.cs @@ -88,17 +88,48 @@ public async Task ExecuteAsync(CancellationToken cancellationToken = defaul // name-only check above reports OK on precisely the condition an operator opens this command // to diagnose: a failed index does not stop queries, it drops them to full scans, and the // only symptom is unexplained slowness. - var failedIndexes = await txRunner.ReadAsync(async runner => + var indexStates = await txRunner.ReadAsync(async runner => { var cursor = await runner.RunAsync(SchemaQueries.ShowIndexStates); var records = await cursor.ToListAsync(); return records - .Where(record => string.Equals( - record["state"].As(), "FAILED", StringComparison.OrdinalIgnoreCase)) - .Select(record => $"{record["name"].As()} ({record["type"].As()})") + .Select(record => new IndexState( + record["name"].As(), + record["state"].As(), + record["type"].As(), + record["populationPercent"].As())) .ToArray(); }, cancellationToken) ?? []; + var failedIndexes = indexStates + .Where(index => string.Equals(index.State, "FAILED", StringComparison.OrdinalIgnoreCase)) + .Select(index => $"{index.Name} ({index.Type})") + .ToArray(); + + // P6. A POPULATING index is neither present-and-healthy nor failed, and it is the state this + // command was least able to describe. It matters most on the VECTOR indexes: a vector search + // against a still-building index succeeds and returns a SUBSET of the corpus, so recall is + // quietly partial and the symptom is "memory seems to have forgotten things" rather than any + // error. Transient by nature, so it is reported rather than failed - but silence here is how + // an operator spends an afternoon debugging retrieval quality on a half-built index. + var populating = indexStates + .Where(index => string.Equals(index.State, "POPULATING", StringComparison.OrdinalIgnoreCase)) + .Select(index => index.PopulationPercent is { } percent + ? FormattableString.Invariant($"{index.Name} ({index.Type}, {percent:0.0}% built)") + : $"{index.Name} ({index.Type})") + .ToArray(); + + if (populating.Length > 0) + { + output.WriteLine( + $"schema-check: note — {populating.Length} index(es) in database '{database}' are still " + + "POPULATING. They are present and not failed, but incomplete: a vector search against a " + + "half-built index returns a subset of the corpus with no error, which reads as memory " + + "having forgotten things. Re-run once they reach ONLINE before judging recall quality:"); + foreach (var descriptor in populating) + output.WriteLine($" - {descriptor}"); + } + // Same helper the bootstrapper uses, so the command and the startup check cannot disagree // about which indexes are ours. var failedOwned = SchemaConformance.SelectOwnedFailures( @@ -171,6 +202,16 @@ public async Task ExecuteAsync(CancellationToken cancellationToken = defaul } } +/// +/// One index as the database reports it: name, lifecycle state, kind, and how far it has populated. +/// +/// +/// A named record rather than a tuple because it crosses the transaction-runner boundary, and the +/// runner is stubbed by return type in tests — an anonymous shape there is unreadable and matches by +/// accident. +/// +public sealed record IndexState(string Name, string State, string Type, double? PopulationPercent); + /// Runs the consolidation / hygiene pass (dry-run unless apply is set). public sealed class ConsolidateCommand(IConsolidationService service, TextWriter output) { diff --git a/tools/AgentMemory.LongMemEval/AgentMemoryLongMemEvalAdapter.cs b/tools/AgentMemory.LongMemEval/AgentMemoryLongMemEvalAdapter.cs index 46cab8c8..dbacd8e0 100644 --- a/tools/AgentMemory.LongMemEval/AgentMemoryLongMemEvalAdapter.cs +++ b/tools/AgentMemory.LongMemEval/AgentMemoryLongMemEvalAdapter.cs @@ -924,6 +924,45 @@ _chatClient is LongMemEvalChatCallMeter callMeter internal static bool CanScoreEmptyRetrieval(LongMemEvalGraphSnapshot? graphSnapshot) => graphSnapshot is { TotalLearned: > 0, CompleteProvenance: true }; + /// + /// The strongest similarity any searched section achieved, or null when nothing was searched. + /// + /// + /// + /// Max across sections rather than facts alone: an answer can arrive through any tier, so a + /// fact-only signal would score a question answered from a retrieved message as unsupported. + /// + /// + /// A section that was searched and came back empty contributes its own minimum-score floor, + /// not zero and not nothing. It is evidence — "we looked at this threshold and found nothing + /// above it" — and dropping it would remove exactly the observations an abstention policy exists + /// to act on, leaving the AUC measured only over questions where retrieval already succeeded. + /// + /// + /// Null when IncludeDiagnostics was off, never 0: a signal that was not collected must not + /// be indistinguishable from one that scored badly. + /// + /// + private static double? SufficiencySignalOf(MemoryContext? context) + { + if (context is null) return null; + + double? best = null; + void Consider(MemoryContextSectionDiagnostics? diagnostics) + { + if (diagnostics is not { Searched: true }) return; + var score = diagnostics.TopScore ?? diagnostics.MinimumScore; + if (best is null || score > best) best = score; + } + + Consider(context.RelevantMessages.Diagnostics); + Consider(context.RelevantEntities.Diagnostics); + Consider(context.RelevantPreferences.Diagnostics); + Consider(context.RelevantFacts.Diagnostics); + Consider(context.SimilarTraces.Diagnostics); + return best; + } + private void RecordTelemetry( int questionNumber, int messagesStored, @@ -975,6 +1014,11 @@ private void RecordTelemetry( RetrievedGoldCoverage = retrievedGoldCoverage, RelationCompleteness = relationCompleteness, AnswerPresence = answerPresence, + // 4.2. The scalar the abstention story rests on: how confident retrieval was that it + // found anything. Paired against AnswerPresence at report time to ask whether the + // signal ORDERS answerable above unanswerable at all -- the question no calibration + // work can skip and none of it has ever asked. + SufficiencySignal = SufficiencySignalOf(context), QuestionType = questionType, // Makes "expansion had nothing to expand" visible per question, instead of // requiring the lexicon to be consulted by hand after a run. @@ -1669,6 +1713,18 @@ public sealed record LongMemEvalQuestionTelemetry( /// public LongMemEvalAnswerPresenceResult? AnswerPresence { get; init; } + /// + /// How confident retrieval was that it found anything: the strongest similarity any searched + /// section achieved, with a searched-but-empty section contributing its threshold floor. + /// + /// + /// Null means diagnostics were not collected — never "scored zero". Paired against + /// it answers PLAN 4.2: does this signal order answerable questions + /// above unanswerable ones? An AUC near 0.5 kills every abstention and calibration story built on + /// it, which is why it is worth measuring before any of them is built. + /// + public double? SufficiencySignal { get; init; } + /// /// The benchmark's own question type, carried so the gate can be read per type. /// diff --git a/tools/AgentMemory.LongMemEval/LongMemEvalAblation.cs b/tools/AgentMemory.LongMemEval/LongMemEvalAblation.cs new file mode 100644 index 00000000..d99ce7f2 --- /dev/null +++ b/tools/AgentMemory.LongMemEval/LongMemEvalAblation.cs @@ -0,0 +1,136 @@ +namespace AgentMemory.LongMemEval; + +/// A product capability that can be switched off to measure what it contributes. +/// Stable identifier, recorded with the result. +/// The memory type it is expected to serve. +/// The option a run sets to disable it, named so a report is reproducible. +internal sealed record LongMemEvalCapability(string Name, string MemoryType, string Option) +{ + /// Assistant-turn capture — the episodic writer. + internal static LongMemEvalCapability Episodic { get; } = + new("episodic-capture", "episodic", "ExtractionOptions.AssistantContentMode"); + + /// Valid-time gating on live recall plus the extractor that writes the bounds. + internal static LongMemEvalCapability Prospective { get; } = + new("valid-time", "temporal", "RecallOptions.ValidTime + LlmExtractionOptions.TemporalValidity"); + + /// Reasoning-trace persistence and recall. + internal static LongMemEvalCapability Traces { get; } = + new("reasoning-traces", "procedural", "AgentFrameworkOptions.PersistReasoningTraces"); + + internal static IReadOnlyList All { get; } = + [Episodic, Prospective, Traces]; +} + +/// One question's behaviour across an ablation pair. +internal sealed record LongMemEvalQuestionFlip( + string QuestionId, + string MemoryType, + bool CorrectWithCapability, + bool CorrectWithout) +{ + /// The capability turned a wrong answer right — the case it is meant to produce. + public bool Gained => CorrectWithCapability && !CorrectWithout; + + /// The capability turned a right answer wrong — the case nobody reports. + public bool Lost => !CorrectWithCapability && CorrectWithout; +} + +/// Result of ablating one capability. +internal sealed record LongMemEvalAblationResult( + LongMemEvalCapability Capability, + IReadOnlyList Flips, + int QuestionsCompared) +{ + public IReadOnlyList Gains => Flips.Where(f => f.Gained).ToArray(); + public IReadOnlyList Losses => Flips.Where(f => f.Lost).ToArray(); + + /// Net questions gained. Can be negative, and that must be reportable. + public int Net => Gains.Count - Losses.Count; + + /// Net movement in accuracy points over the compared set. + public double NetPoints => QuestionsCompared == 0 ? 0 : Net * 100.0 / QuestionsCompared; +} + +/// +/// Compares a run with a capability on against the same run with it off. +/// +/// +/// +/// Per-question, not aggregate, deliberately. An aggregate hides the two things worth knowing: +/// which questions a capability rescued, and whether it broke any that previously worked. A +/// capability that gains three and loses three reports as neutral and is not — it is churn, and churn +/// is the signature of a change that is moving noise rather than adding signal. +/// +/// +/// No vendor in the surveyed field publishes a per-memory-type ablation. That is the whole +/// reason this exists: the differentiation available here is the measurement, not the tier. +/// +/// +/// This does not decide anything on its own. Its output must be read against the per-type noise +/// floor — a net movement smaller than what the same configuration already varied by is not a result. +/// +/// +internal static class LongMemEvalAblation +{ + /// + /// Flips between the two arms, restricted to questions present and judged in BOTH. + /// + /// + /// A question judged in only one arm cannot be compared, and including it would let a run that + /// simply answered fewer questions look like a capability effect. + /// + internal static LongMemEvalAblationResult Compare( + LongMemEvalCapability capability, + IReadOnlyDictionary withCapability, + IReadOnlyDictionary withoutCapability, + IReadOnlyDictionary memoryTypeByQuestionId) + { + ArgumentNullException.ThrowIfNull(capability); + ArgumentNullException.ThrowIfNull(withCapability); + ArgumentNullException.ThrowIfNull(withoutCapability); + ArgumentNullException.ThrowIfNull(memoryTypeByQuestionId); + + var flips = new List(); + var compared = 0; + + foreach (var (questionId, correctWith) in withCapability) + { + if (!withoutCapability.TryGetValue(questionId, out var correctWithout)) continue; + compared++; + if (correctWith == correctWithout) continue; + + flips.Add(new LongMemEvalQuestionFlip( + questionId, + memoryTypeByQuestionId.TryGetValue(questionId, out var type) + ? type + : LongMemEvalMemoryTypeMap.Unmapped, + correctWith, + correctWithout)); + } + + return new LongMemEvalAblationResult( + capability, + flips.OrderBy(f => f.QuestionId, StringComparer.Ordinal).ToArray(), + compared); + } + + /// + /// Whether the movement survives the type's own measured spread. + /// + /// + /// Reported rather than applied: an ablation whose effect is inside the noise floor is a null + /// result, and a null result is publishable — it is how a component that does not earn its tokens + /// gets found in-house rather than by a competitor. + /// + internal static bool SurvivesNoiseFloor( + LongMemEvalAblationResult result, + IReadOnlyList noiseFloors) + { + ArgumentNullException.ThrowIfNull(result); + ArgumentNullException.ThrowIfNull(noiseFloors); + + var floor = noiseFloors.FirstOrDefault(f => f.MemoryType == result.Capability.MemoryType); + return floor is not null && floor.Separates(Math.Abs(result.NetPoints)); + } +} diff --git a/tools/AgentMemory.LongMemEval/LongMemEvalMemoryTypeMap.cs b/tools/AgentMemory.LongMemEval/LongMemEvalMemoryTypeMap.cs index b8f42ffd..d72791f9 100644 --- a/tools/AgentMemory.LongMemEval/LongMemEvalMemoryTypeMap.cs +++ b/tools/AgentMemory.LongMemEval/LongMemEvalMemoryTypeMap.cs @@ -35,14 +35,29 @@ internal sealed class LongMemEvalMemoryTypeMap /// The mapping revision, so a per-type figure can name the opinion it came from. internal string Revision { get; } + /// + /// The task label → memory types mapping, exposed so a selection can be derived from the + /// same data the reports use rather than from a second copy of the taxonomy that drifts. + /// + internal IReadOnlyDictionary> TaskTypes => _byTaskType; + + /// + /// Every memory type the mapping names, including ones no task label reaches (metamemory arrives + /// through abstention; procedural is not in this dataset at all). Used to explain a rejected + /// selection, so an unreachable type reads as "not here" rather than "misspelled". + /// + internal IReadOnlyList KnownMemoryTypes { get; } + private LongMemEvalMemoryTypeMap( string revision, IReadOnlyDictionary> byTaskType, - IReadOnlyList abstentionTypes) + IReadOnlyList abstentionTypes, + IReadOnlyList knownMemoryTypes) { Revision = revision; _byTaskType = byTaskType; _abstentionTypes = abstentionTypes; + KnownMemoryTypes = knownMemoryTypes; } /// The embedded mapping. @@ -63,10 +78,20 @@ private static LongMemEvalMemoryTypeMap Load() pair => (IReadOnlyList)pair.Value.MemoryTypes, StringComparer.OrdinalIgnoreCase); + // Declared names first, so a type the document names but no task label reaches -- procedural, + // metamemory -- is still reportable as known-but-unreachable rather than as a typo. + var known = document.MemoryTypes.Keys + .Concat(byTask.Values.SelectMany(types => types)) + .Concat(document.Abstention?.MemoryTypes ?? [MetaMemory]) + .Distinct(StringComparer.OrdinalIgnoreCase) + .OrderBy(type => type, StringComparer.Ordinal) + .ToArray(); + return new LongMemEvalMemoryTypeMap( document.Revision, byTask, - document.Abstention?.MemoryTypes ?? [MetaMemory]); + document.Abstention?.MemoryTypes ?? [MetaMemory], + known); } private static readonly JsonSerializerOptions SerializerOptions = new() @@ -93,6 +118,7 @@ internal IReadOnlyList ForQuestion(string? taskType, bool isAbstention) private sealed class MapDocument { [JsonPropertyName("revision")] public string Revision { get; set; } = "unknown"; + [JsonPropertyName("memoryTypes")] public Dictionary MemoryTypes { get; set; } = []; [JsonPropertyName("taskTypes")] public Dictionary TaskTypes { get; set; } = []; [JsonPropertyName("abstention")] public TaskTypeEntry? Abstention { get; set; } } diff --git a/tools/AgentMemory.LongMemEval/LongMemEvalMemoryTypeSelection.cs b/tools/AgentMemory.LongMemEval/LongMemEvalMemoryTypeSelection.cs new file mode 100644 index 00000000..e397f004 --- /dev/null +++ b/tools/AgentMemory.LongMemEval/LongMemEvalMemoryTypeSelection.cs @@ -0,0 +1,74 @@ +namespace AgentMemory.LongMemEval; + +/// +/// Turns a request for memory types into the dataset task labels that exercise them. +/// +/// +/// +/// This is the inverse of , and it exists because a per-type +/// claim needs a per-type sample, not a per-type slice of a mixed one. A 50-question stratified +/// sample yields roughly 6 single-session-assistant questions; on 6 questions one item is worth +/// 16.7 points, while two runs of an identical configuration have been measured 25 points apart +/// on 50. A subset that small cannot decide anything, so reporting it as a per-type accuracy would +/// publish noise with a decimal point on it. +/// +/// +/// Selecting the type first is what fixes that: 50 questions of one type gives that type the same +/// denominator the aggregate has always had. +/// +/// +/// Derived from the same embedded mapping the reports use, never from a second hardcoded list. +/// Two copies of a taxonomy drift, and the failure would be silent — a run would sample one set of +/// labels and report per-type figures computed from another, with both looking correct. +/// +/// +internal static class LongMemEvalMemoryTypeSelection +{ + /// + /// The dataset task labels whose questions exercise any of . + /// + /// + /// Returns for an empty request, which is the sampler's "no filter" value + /// and reproduces the stratified-across-all-types default exactly. + /// + /// + /// A named type no task label reaches. That is a hard failure on purpose: the alternative is a run + /// that quietly samples everything and reports it as if it were the requested type. Procedural is + /// the live case — LongMemEval-S contains no procedural questions at any sample size, so + /// asking for it must say so rather than return an empty or unfiltered selection. + /// + internal static IReadOnlyList? TaskTypesFor( + IReadOnlyList memoryTypes, LongMemEvalMemoryTypeMap? map = null) + { + ArgumentNullException.ThrowIfNull(memoryTypes); + if (memoryTypes.Count == 0) return null; + + map ??= LongMemEvalMemoryTypeMap.Default; + + var requested = new HashSet(memoryTypes, StringComparer.OrdinalIgnoreCase); + var selected = new List(); + var reached = new HashSet(StringComparer.OrdinalIgnoreCase); + + foreach (var (taskType, types) in map.TaskTypes) + { + if (!types.Any(requested.Contains)) continue; + selected.Add(taskType); + foreach (var type in types.Where(requested.Contains)) reached.Add(type); + } + + var unreachable = requested.Where(type => !reached.Contains(type)) + .OrderBy(type => type, StringComparer.Ordinal).ToList(); + if (unreachable.Count > 0) + { + throw new ArgumentException( + $"No LongMemEval task label exercises: {string.Join(", ", unreachable)}. " + + $"Known memory types are: {string.Join(", ", map.KnownMemoryTypes)}. " + + "Abstention questions are the only source of metamemory and are selected with " + + "--abstention rather than by task label; LongMemEval-S contains no procedural " + + "questions at all, so no sample of it can measure a procedural tier."); + } + + selected.Sort(StringComparer.Ordinal); + return selected; + } +} diff --git a/tools/AgentMemory.LongMemEval/LongMemEvalSufficiencyAuc.cs b/tools/AgentMemory.LongMemEval/LongMemEvalSufficiencyAuc.cs new file mode 100644 index 00000000..9350263c --- /dev/null +++ b/tools/AgentMemory.LongMemEval/LongMemEvalSufficiencyAuc.cs @@ -0,0 +1,133 @@ +using System.Globalization; + +namespace AgentMemory.LongMemEval; + +/// +/// Does the retrieval-sufficiency signal actually predict whether the answer was there? +/// +/// +/// +/// Per-section diagnostics now say why a section came back thin — searched and empty, searched +/// and short, top score, threshold. The abstention story downstream assumes those numbers mean +/// something: that a low top score or an empty section indicates the answer was not retrievable. That +/// assumption has never been tested, and if the scores are noise then every calibration, every +/// "I don't know" and every confidence surface built on them is decoration. +/// +/// +/// AUC is the right summary because it needs no threshold. Asking "does topScore < 0.7 +/// predict absence?" answers a question about 0.7; AUC asks whether the signal orders present +/// above absent at all, over every threshold at once. 0.5 is the kill line: a signal that +/// orders no better than a coin has nothing to calibrate, and finding that out costs a few hours +/// rather than a quarter. +/// +/// +/// Computed by the rank formula (Mann–Whitney U), not by trapezoids over a swept threshold — ties are +/// the failure mode here, and they are not incidental. A signal that returns 0 for every empty +/// section produces a large tie block, and a curve-based implementation silently scores those as if +/// they were ordered. The rank form gives ties the 0.5 credit they have earned, so a degenerate signal +/// reports ≈0.5 instead of an accidental 0.8. +/// +/// +internal static class LongMemEvalSufficiencyAuc +{ + /// + /// One question's pairing of the sufficiency signal with whether its answer was actually stored. + /// + /// + /// Higher must mean "more sufficient". The natural signal is the section's top similarity score, + /// with an empty section contributing its floor. + /// + /// Whether the gold answer was found in stored memory at all. + internal readonly record struct Observation(double Signal, bool AnswerPresent); + + /// + /// The AUC of , plus the counts it was computed over. + /// + /// + /// Null AUC when either class is empty. That is not a degenerate 0.5 — it means the question was + /// never asked, and reporting a number for it would be inventing evidence. A run where every + /// answer was present, or none was, cannot say anything about ordering. + /// + internal static LongMemEvalSufficiencyAucResult Compute(IReadOnlyList observations) + { + ArgumentNullException.ThrowIfNull(observations); + + var present = observations.Where(o => o.AnswerPresent).ToArray(); + var absent = observations.Where(o => !o.AnswerPresent).ToArray(); + if (present.Length == 0 || absent.Length == 0) + return new LongMemEvalSufficiencyAucResult(null, present.Length, absent.Length, 0); + + // Mann-Whitney U over mid-ranks. Every present/absent pair contributes 1 when the present one + // scores higher, 0 when lower, and 0.5 when they tie -- which is what stops a signal that is + // constant across most of the corpus from scoring well. + var ranks = MidRanks([.. observations.Select(o => o.Signal)]); + var presentRankSum = 0.0; + var ties = 0; + for (var i = 0; i < observations.Count; i++) + if (observations[i].AnswerPresent) presentRankSum += ranks[i]; + + var distinct = observations.Select(o => o.Signal).Distinct().Count(); + if (distinct < observations.Count) ties = observations.Count - distinct; + + var u = presentRankSum - (present.Length * (present.Length + 1) / 2.0); + var auc = u / ((double)present.Length * absent.Length); + return new LongMemEvalSufficiencyAucResult(auc, present.Length, absent.Length, ties); + } + + private static double[] MidRanks(double[] values) + { + var order = Enumerable.Range(0, values.Length) + .OrderBy(i => values[i]) + .ToArray(); + var ranks = new double[values.Length]; + var position = 0; + while (position < order.Length) + { + var run = position + 1; + while (run < order.Length && values[order[run]].Equals(values[order[position]])) run++; + // 1-based mid-rank shared by the whole tie block. + var mid = (position + run + 1) / 2.0; + for (var i = position; i < run; i++) ranks[order[i]] = mid; + position = run; + } + return ranks; + } +} + +/// The AUC, and everything needed to judge whether to believe it. +/// +/// Null when one class was empty — "not asked", never a default 0.5. +/// +/// Questions whose gold answer was found in memory. +/// Questions whose gold answer was not. +/// +/// How many observations shared a signal value with another. A large figure means the signal is +/// mostly constant, and an AUC near 0.5 is then a property of the signal rather than of retrieval. +/// +internal readonly record struct LongMemEvalSufficiencyAucResult( + double? Auc, int PresentCount, int AbsentCount, int TiedObservations) +{ + /// + /// Whether the signal orders better than a coin by a margin worth building on. + /// + /// + /// The threshold is stated here rather than chosen after seeing the number. 0.6 is deliberately + /// unambitious: it is the level below which an abstention policy cannot beat "always answer" by + /// enough to justify the refusals it would cause. + /// + internal bool JustifiesAbstentionWork => Auc is >= 0.6; + + /// A one-line rendering that always states the denominators. + /// + /// An AUC without its class counts is unreadable: 0.83 over 47 present and 3 absent is three + /// questions' worth of evidence, and the bare number hides that completely. + /// + internal string Describe() => Auc is not { } auc + ? $"AUC not measured (present={PresentCount}, absent={AbsentCount}) - one class was empty, " + + "so the signal was never asked to order anything" + : string.Format( + CultureInfo.InvariantCulture, + "AUC {0:0.000} over {1} present / {2} absent ({3} tied){4}", + auc, PresentCount, AbsentCount, TiedObservations, + JustifiesAbstentionWork ? string.Empty : " - at or below the 0.6 line, abstention work is not justified"); +} diff --git a/tools/AgentMemory.LongMemEval/LongMemEvalSufficiencyReport.cs b/tools/AgentMemory.LongMemEval/LongMemEvalSufficiencyReport.cs new file mode 100644 index 00000000..e32891e7 --- /dev/null +++ b/tools/AgentMemory.LongMemEval/LongMemEvalSufficiencyReport.cs @@ -0,0 +1,52 @@ +namespace AgentMemory.LongMemEval; + +/// +/// Pairs each question's sufficiency signal with whether its answer was stored, and reports the AUC. +/// +/// +/// +/// The pairing is where this can quietly go wrong, so the exclusions are explicit rather than +/// incidental. A question contributes only when both halves are real: a signal that was +/// actually collected, and a presence verdict that was actually checkable. Substituting a default for +/// either — 0 for an uncollected signal, "absent" for an uncheckable answer — would populate the AUC +/// with observations that carry no information and drag it towards 0.5, which is the kill line. A +/// metric computed over absent data is a metric that can lie. +/// +/// +/// The excluded counts travel with the result for the same reason: an AUC over 6 of 50 questions is +/// not the run's AUC, and the bare number cannot say so. +/// +/// +internal static class LongMemEvalSufficiencyReport +{ + internal static object From(IReadOnlyList telemetry) + { + ArgumentNullException.ThrowIfNull(telemetry); + + var usable = telemetry + .Where(item => item.SufficiencySignal is not null && item.AnswerPresence is { Checkable: true }) + .ToArray(); + + var result = LongMemEvalSufficiencyAuc.Compute( + [.. usable.Select(item => new LongMemEvalSufficiencyAuc.Observation( + item.SufficiencySignal!.Value, item.AnswerPresence!.Present))]); + + return new + { + auc = result.Auc, + presentCount = result.PresentCount, + absentCount = result.AbsentCount, + tiedObservations = result.TiedObservations, + justifiesAbstentionWork = result.JustifiesAbstentionWork, + summary = result.Describe(), + // Every question that could not contribute, and why. Without these the AUC's denominator + // is unauditable -- and it is the denominator, not the number, that decides whether the + // result means anything. + excludedNoSignal = telemetry.Count(item => item.SufficiencySignal is null), + excludedNotCheckable = telemetry.Count( + item => item.SufficiencySignal is not null + && item.AnswerPresence is null or { Checkable: false }), + questionsConsidered = telemetry.Count, + }; + } +} diff --git a/tools/AgentMemory.LongMemEval/LongMemEvalTypedNoiseFloor.cs b/tools/AgentMemory.LongMemEval/LongMemEvalTypedNoiseFloor.cs new file mode 100644 index 00000000..1e4e706e --- /dev/null +++ b/tools/AgentMemory.LongMemEval/LongMemEvalTypedNoiseFloor.cs @@ -0,0 +1,102 @@ +namespace AgentMemory.LongMemEval; + +/// Repeat-variance of one memory type's accuracy across runs of the same arm. +/// The type. +/// How many runs contributed. +/// Questions of this type per run (they must agree across runs). +/// Lowest accuracy observed. +/// Highest accuracy observed. +/// Mean accuracy. +/// Sample standard deviation, or null below two runs. +internal sealed record LongMemEvalTypedNoiseFloor( + string MemoryType, + int Runs, + int Questions, + double MinAccuracy, + double MaxAccuracy, + double MeanAccuracy, + double? StandardDeviation) +{ + /// Observed spread in accuracy points — the crudest honest band. + public double RangePoints => (MaxAccuracy - MinAccuracy) * 100.0; + + /// What a single question is worth, in accuracy points. + public double PointsPerQuestion => Questions == 0 ? 0 : 100.0 / Questions; + + /// + /// Whether a difference of is larger than the observed spread. + /// + /// + /// Deliberately crude and deliberately conservative. It is not a significance test: with a handful + /// of runs the honest question is only "is this bigger than what the same configuration already + /// varied by?", and a difference that fails that has no business being reported as a result. + /// A single run yields no spread, so nothing is separable — which is the correct answer, not a + /// missing feature. + /// + public bool Separates(double points) => Runs >= 2 && points > RangePoints; +} + +/// +/// Measures how much a memory type's accuracy varies across repeats of the same configuration. +/// +/// +/// +/// Why this must exist before any per-type number is published. A per-type subset is small — +/// episodic is 6 of 50 questions, i.e. 16.7 accuracy points per question — and the whole-run +/// band (~±9 points at n=50) does not transfer to it. Quoting a per-type figure without its own band +/// is how a table invites exactly the comparison it cannot support. +/// +/// +/// It also cannot be inferred from question count alone: extraction on this deployment is +/// non-deterministic, so two builds of one configuration differ in what they stored, and the spread +/// that matters is the measured one rather than a binomial estimate. +/// +/// +internal static class LongMemEvalTypedNoiseFloorCalculator +{ + /// + /// Per-type spread across runs. must be repeats of the SAME arm and + /// configuration; mixing arms measures the difference between them, not the noise within one. + /// + internal static IReadOnlyList Measure( + IReadOnlyList> runs) + { + ArgumentNullException.ThrowIfNull(runs); + if (runs.Count == 0) return []; + + var byType = new Dictionary>(StringComparer.Ordinal); + foreach (var run in runs) + foreach (var row in run) + { + if (!byType.TryGetValue(row.MemoryType, out var list)) + byType[row.MemoryType] = list = []; + list.Add(row); + } + + var results = new List(byType.Count); + foreach (var (type, rows) in byType) + { + // A type absent from some runs cannot have its spread measured against the others: the + // denominators differ, so the comparison is between different questions. Reported at the + // count it actually has rather than silently averaged over a changing set. + var accuracies = rows.Where(r => r.Accuracy is not null).Select(r => r.Accuracy!.Value).ToArray(); + if (accuracies.Length == 0) continue; + + var mean = accuracies.Average(); + double? sd = accuracies.Length < 2 + ? null + : Math.Sqrt(accuracies.Sum(a => (a - mean) * (a - mean)) / (accuracies.Length - 1)); + + results.Add(new LongMemEvalTypedNoiseFloor( + type, + accuracies.Length, + rows[0].Questions, + accuracies.Min(), + accuracies.Max(), + mean, + sd)); + } + + return results.OrderByDescending(r => r.Questions).ThenBy(r => r.MemoryType, StringComparer.Ordinal).ToArray(); + } +} diff --git a/tools/AgentMemory.LongMemEval/LongMemEvalTypedReport.cs b/tools/AgentMemory.LongMemEval/LongMemEvalTypedReport.cs new file mode 100644 index 00000000..1b26a30c --- /dev/null +++ b/tools/AgentMemory.LongMemEval/LongMemEvalTypedReport.cs @@ -0,0 +1,136 @@ +using System.Globalization; +using System.Text; + +namespace AgentMemory.LongMemEval; + +/// +/// Renders the per-memory-type table, with the caveats a reader needs attached to the numbers. +/// +/// +/// +/// The integrity rules this project already adopted are enforced here, in code, rather than +/// left to whoever writes the surrounding prose — because the failure mode being guarded against is a +/// number that travels without its context. A 57.7k-star project's headline died on exactly that. +/// +/// +/// Every figure carries its question count and what one question is worth. +/// A figure with no measured band is labelled as having none, never quoted bare. +/// An ablation inside the noise floor is printed as no result, not as a small win. +/// Types the dataset cannot reach are listed, so absence never reads as a zero. +/// The mapping revision is printed, because the grouping is an opinion. +/// +/// +internal static class LongMemEvalTypedReport +{ + /// Markdown for a per-type breakdown, with bands when they have been measured. + internal static string Render( + IReadOnlyList rows, + IReadOnlyList? noiseFloors = null, + IReadOnlyList? ablations = null, + string? mappingRevision = null) + { + ArgumentNullException.ThrowIfNull(rows); + var sb = new StringBuilder(); + var invariant = CultureInfo.InvariantCulture; + + sb.AppendLine("## Accuracy by memory type"); + sb.AppendLine(); + sb.AppendLine("| memory type | questions | accuracy | 1 question = | measured band | extraction-side | retrieval-side | unattributable |"); + sb.AppendLine("|---|---:|---:|---:|---:|---:|---:|---:|"); + + foreach (var row in rows) + { + var floor = noiseFloors?.FirstOrDefault(f => f.MemoryType == row.MemoryType); + var band = floor is null || floor.Runs < 2 + ? "**not measured**" + : string.Format(invariant, "±{0:F1} pts", floor.RangePoints / 2); + + sb.AppendLine(string.Format( + invariant, + "| {0} | {1} | {2} | {3:F1} pts | {4} | {5} | {6} | {7} |", + row.MemoryType, + row.Questions, + row.Accuracy is { } a ? string.Format(invariant, "{0:P1}", a) : "—", + row.PointsPerQuestion ?? 0, + band, + row.ExtractionFailures, + row.RetrievalFailures, + row.Unattributable)); + } + + sb.AppendLine(); + if (noiseFloors is null || noiseFloors.All(f => f.Runs < 2)) + { + sb.AppendLine( + "> ⚠️ **No band has been measured.** These figures come from a single run, so none of " + + "them supports a comparison — with another configuration, or with anyone else's " + + "published number. Repeats are what make a per-type figure quotable."); + sb.AppendLine(); + } + + if (ablations is { Count: > 0 }) + { + sb.AppendLine("## What each capability contributed"); + sb.AppendLine(); + sb.AppendLine("| capability | disabled via | gained | lost | net | verdict |"); + sb.AppendLine("|---|---|---:|---:|---:|---|"); + + foreach (var ablation in ablations) + { + var survives = noiseFloors is not null + && LongMemEvalAblation.SurvivesNoiseFloor(ablation, noiseFloors); + + // Inside the band is a NULL RESULT and is printed as one. A component that does not + // earn its tokens should be found here rather than by a competitor. + var verdict = survives + ? string.Format(invariant, "**{0:+0.0;-0.0} pts**", ablation.NetPoints) + : "no result — inside the band"; + + sb.AppendLine(string.Format( + invariant, + "| {0} | `{1}` | {2} | {3} | {4:+0;-0;0} | {5} |", + ablation.Capability.Name, + ablation.Capability.Option, + ablation.Gains.Count, + ablation.Losses.Count, + ablation.Net, + verdict)); + } + + sb.AppendLine(); + // Churn is invisible in a net figure and is the signature of a change moving noise. + var churn = ablations.Where(a => a.Gains.Count > 0 && a.Losses.Count > 0).ToArray(); + if (churn.Length > 0) + { + sb.AppendLine( + "> **Churn:** " + string.Join(", ", churn.Select(a => + $"`{a.Capability.Name}` gained {a.Gains.Count} and lost {a.Losses.Count}")) + + ". A capability that rescues and breaks in similar measure is moving answers " + + "around rather than adding signal, and the net alone would hide that."); + sb.AppendLine(); + } + } + + sb.AppendLine("## What this dataset cannot measure"); + sb.AppendLine(); + sb.AppendLine( + "**Procedural memory is unreachable here at any sample size.** LongMemEval-S is chat QA: " + + "no build commands, no tool invocations, no fix trajectories. A score from this dataset " + + "is not evidence about a procedural tier, and quoting one as though it were is the " + + "metric substitution this report exists to avoid."); + sb.AppendLine(); + sb.AppendLine( + "**Meta-memory is reachable only through abstention questions**, so a run that sampled " + + "none has not measured it — a missing row means unmeasured, never zero."); + + if (!string.IsNullOrWhiteSpace(mappingRevision)) + { + sb.AppendLine(); + sb.AppendLine( + $"_Task-label → memory-type mapping revision `{mappingRevision}`. The grouping is an " + + "opinion; a figure computed from a different revision is a different figure._"); + } + + return sb.ToString(); + } +} diff --git a/tools/AgentMemory.LongMemEval/Program.cs b/tools/AgentMemory.LongMemEval/Program.cs index 80a69112..ebaca58f 100644 --- a/tools/AgentMemory.LongMemEval/Program.cs +++ b/tools/AgentMemory.LongMemEval/Program.cs @@ -105,7 +105,8 @@ public static async Task RunAsync(string[] args) options.Seed, options.JudgeRetryAttempts, options.EvidenceDetail, - options.MaxRelevantMessages); + options.MaxRelevantMessages, + includeQuestionTypes: LongMemEvalMemoryTypeSelection.TaskTypesFor(options.MemoryTypes)); var evidenceIndex = LongMemEvalEvidenceIndex.Load( options.DatasetPath, benchmarkOptions); @@ -209,6 +210,16 @@ await File.ReadAllBytesAsync(options.DatasetPath).ConfigureAwait(false))), questions = options.Questions, seed = options.Seed, stratified = true, + // A typed run answers a different question from an all-types run on the same seed + // and count, so it must never read as comparable to one. The mapping revision + // travels with it because the selection is an opinion that will be revised, and a + // per-type figure that cannot name the taxonomy it came from is unauditable. + memoryTypes = options.MemoryTypes.Count == 0 + ? "all" + : string.Join(",", options.MemoryTypes.OrderBy(t => t, StringComparer.Ordinal)), + memoryTypeMapRevision = options.MemoryTypes.Count == 0 + ? null + : LongMemEvalMemoryTypeMap.Default.Revision, answerModel = deployment, judgeModel = deployment, maxRelevantMessages = options.MaxRelevantMessages, @@ -260,7 +271,13 @@ await File.ReadAllBytesAsync(options.DatasetPath).ConfigureAwait(false))), totalPreferencesRetrieved = adapter.QuestionTelemetry.Sum(item => item.PreferencesRetrieved), graphRagQuestions = adapter.QuestionTelemetry.Count(item => item.GraphRagIncluded), zeroStoreQuestions = adapter.QuestionTelemetry.Count(item => item.MessagesStored == 0), - zeroRecallQuestions = adapter.QuestionTelemetry.Count(item => item.ItemsRetrieved == 0) + zeroRecallQuestions = adapter.QuestionTelemetry.Count(item => item.ItemsRetrieved == 0), + // PLAN 4.2. Does the sufficiency signal ORDER answerable questions above + // unanswerable ones? Emitted on every run because it costs nothing -- no extra + // call, no rebuild, it reads two fields already recorded -- and because a number + // that appears only when someone remembers to ask for it never gets asked for. + // Null auc means one class was empty and the signal was never put to the test. + sufficiencyAuc = LongMemEvalSufficiencyReport.From(adapter.QuestionTelemetry), }, callAccounting = new { @@ -356,7 +373,7 @@ await File.WriteAllTextAsync( "--chronological-context", "--dataset", "--evidence-detail", "--exclude-synthetic-messages", "--judge-retries", "--max-items-per-session", "--max-relevant", "--memory-mode", "--oracle", "--output", "--questions", "--seed", - "--units", "--turns", "--repeat", "--extraction-seed", + "--units", "--turns", "--repeat", "--extraction-seed", "--memory-types", ]; private static Options Parse(string[] args) @@ -384,7 +401,8 @@ private static Options Parse(string[] args) Value("--output"), Array.IndexOf(args, "--exclude-synthetic-messages") >= 0, ParseNonNegative(Value("--max-items-per-session"), 0, "--max-items-per-session"), - Array.IndexOf(args, "--chronological-context") >= 0); + Array.IndexOf(args, "--chronological-context") >= 0, + ParseMemoryTypes(Value("--memory-types"))); } private static object Project(LongMemEvalChatCallSnapshot snapshot) => new @@ -432,6 +450,30 @@ private static LongMemEvalMemoryMode ParseMemoryMode(string? value) => "--memory-mode must be one of: raw, structured, hybrid.") }; + /// + /// Parses --memory-types episodic,temporal into the requested memory types. + /// + /// + /// Empty means "every type", which is the sampling this harness has always done and the value + /// every sealed base was recorded under. The selection is turned into task labels by + /// , from the same embedded mapping the per-type + /// reports use -- a second hardcoded list here would drift, and the run would then sample one set + /// of labels while reporting per-type figures computed from another. + /// + private static IReadOnlyList ParseMemoryTypes(string? value) + { + if (string.IsNullOrWhiteSpace(value)) return []; + var types = value + .Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries) + .ToArray(); + if (types.Length == 0) + throw new ArgumentException("--memory-types requires at least one memory type."); + // Validated here, before any container starts or any provider call is made: an unreachable + // type must stop the run rather than silently widen it back to the full sample. + LongMemEvalMemoryTypeSelection.TaskTypesFor(types); + return types; + } + private static int ParseNonNegative(string? value, int defaultValue, string option) { if (value is null) return defaultValue; @@ -476,7 +518,15 @@ dotnet run --project tools/AgentMemory.LongMemEval -- \ [--provider-no-progress-timeout-seconds 600] \ [--evidence-detail none|identifiers|content] \ [--exclude-synthetic-messages] [--max-items-per-session N] [--chronological-context] \ - [--oracle none|failed|all] [--judge-retries 2] [--output ] + [--memory-types episodic,temporal] [--oracle none|failed|all] [--judge-retries 2] [--output ] + + --memory-types samples ONLY questions exercising the named memory types, so a per-type claim + gets a per-type denominator. A 50-question stratified sample yields ~6 single-session-assistant + questions; on 6, one item is 16.7 points, while two runs of an IDENTICAL config have measured + 25 points apart on 50 -- so a slice that small can only publish noise. Default: every type, + which is the sampling every sealed base was recorded under. Types: semantic, episodic, + temporal. metamemory arrives via abstention questions rather than a task label, and + LongMemEval-S contains no procedural questions at any sample size. --exclude-synthetic-messages over-fetches 3x the message budget, drops only AgentEval's formatter boilerplate (session boundaries and padding), keeps retrieval order, and selects @@ -520,5 +570,6 @@ private sealed record Options( string? OutputPath, bool ExcludeSyntheticMessages, int MaxItemsPerSourceSession, - bool ChronologicalAnswerContext); + bool ChronologicalAnswerContext, + IReadOnlyList MemoryTypes); } diff --git a/tools/AgentMemory.McpHost/AgentMemory.McpHost.csproj b/tools/AgentMemory.McpHost/AgentMemory.McpHost.csproj new file mode 100644 index 00000000..69afd86e --- /dev/null +++ b/tools/AgentMemory.McpHost/AgentMemory.McpHost.csproj @@ -0,0 +1,41 @@ + + + + Exe + AgentMemory.McpHost + + + true + agent-memory-mcp + AgentMemory.McpHost + DotnetTool + Turnkey MCP server for AgentMemory for .NET — runs the 25 memory tools over stdio or HTTP against Neo4j, configured entirely by environment variables. Install with `dotnet tool install -g AgentMemory.McpHost`. + agent-memory;mcp;model-context-protocol;ai;memory;neo4j;dotnet-tool + + + true + + + + + + + + + + + + + + + + + + + + + diff --git a/tools/AgentMemory.McpHost/Dockerfile b/tools/AgentMemory.McpHost/Dockerfile new file mode 100644 index 00000000..07822bc0 --- /dev/null +++ b/tools/AgentMemory.McpHost/Dockerfile @@ -0,0 +1,34 @@ +# Build the MCP host as a self-contained container image. +# +# Built from the repository root so the ProjectReferences resolve: +# docker build -f tools/AgentMemory.McpHost/Dockerfile -t agent-memory-mcp . +# +# The image defaults to the HTTP transport. stdio is for a client that launches the process itself, +# which is the `dotnet tool install -g` path, not this one -- a container speaking stdio would need +# `docker run -i` and an attached client, and getting that subtly wrong presents as a silent hang. +FROM mcr.microsoft.com/dotnet/sdk:9.0 AS build +WORKDIR /src + +COPY Directory.Build.props ./ +COPY README.md ./ +COPY resources/ ./resources/ +COPY src/ ./src/ +COPY tools/AgentMemory.McpHost/ ./tools/AgentMemory.McpHost/ + +RUN dotnet publish tools/AgentMemory.McpHost/AgentMemory.McpHost.csproj \ + -c Release -o /app --no-self-contained + +FROM mcr.microsoft.com/dotnet/aspnet:9.0 +WORKDIR /app +COPY --from=build /app . + +# Bind to every interface: localhost inside a container is reachable only from inside it, which is the +# most common way a correctly built image looks broken from the host. +ENV AGENT_MEMORY_MCP_TRANSPORT=http \ + AGENT_MEMORY_MCP_URL=http://0.0.0.0:5233 +EXPOSE 5233 + +# Non-root by default. This process needs no write access to its own filesystem. +USER $APP_UID + +ENTRYPOINT ["dotnet", "AgentMemory.McpHost.dll"] diff --git a/tools/AgentMemory.McpHost/McpHostOptions.cs b/tools/AgentMemory.McpHost/McpHostOptions.cs new file mode 100644 index 00000000..d2aefbba --- /dev/null +++ b/tools/AgentMemory.McpHost/McpHostOptions.cs @@ -0,0 +1,200 @@ +using System.Globalization; +using AgentMemory.McpServer; +using Microsoft.Extensions.Logging; + +namespace AgentMemory.McpHost; + +/// Which transport the server speaks. +internal enum McpHostTransport +{ + /// JSON-RPC over stdin/stdout — what a desktop MCP client launches. + Stdio = 0, + + /// Streamable HTTP — one server, many clients, reachable from a container. + Http = 1, +} + +/// +/// Everything the host needs, resolved from environment variables with command-line overrides. +/// +/// +/// +/// Environment first. A desktop MCP client launches the server through a config file whose +/// env block is the natural place for credentials; a container gets them from the orchestrator. +/// Flags exist so the same binary can be driven by hand without exporting anything, and they win when +/// both are present, because a flag is the more specific and more deliberate statement. +/// +/// +/// Parsing is separated from running so it is testable without a database, a provider or a port — the +/// defaults and the precedence rules are the part most likely to be quietly wrong, and the part a +/// runtime smoke test would never distinguish from a working server. +/// +/// +internal sealed record McpHostOptions +{ + internal required string AzureEndpoint { get; init; } + internal required string AzureApiKey { get; init; } + internal required string EmbeddingDeployment { get; init; } + + internal required string Neo4jUri { get; init; } + internal required string Neo4jUsername { get; init; } + internal required string Neo4jPassword { get; init; } + internal required string Neo4jDatabase { get; init; } + + internal McpHostTransport Transport { get; init; } + internal string HttpUrl { get; init; } = "http://localhost:5233"; + internal string ServerName { get; init; } = "agent-memory"; + internal bool ReadOnly { get; init; } + internal bool EnableGraphQuery { get; init; } + internal bool Bootstrap { get; init; } = true; + internal LogLevel LogLevel { get; init; } = LogLevel.Information; + + private static readonly string[] Known = + [ + "--transport", "--url", "--server-name", "--read-only", "--enable-graph-query", + "--no-bootstrap", "--log-level", "--help", "-h", "/?", + ]; + + internal static McpHostOptions Parse(string[] args, Func environment) + { + ArgumentNullException.ThrowIfNull(args); + ArgumentNullException.ThrowIfNull(environment); + + // An unknown flag is an error, never ignored. A typo in --read-only would otherwise start a + // fully writable server that the operator believes is read-only -- the one failure this host + // must not have. + foreach (var argument in args.Where(a => a.StartsWith("--", StringComparison.Ordinal))) + if (!Known.Contains(argument, StringComparer.Ordinal)) + throw new ArgumentException($"unknown option '{argument}'."); + + string? Value(string name) + { + var index = Array.IndexOf(args, name); + if (index < 0) return null; + if (index + 1 >= args.Length) throw new ArgumentException($"{name} requires a value."); + return args[index + 1]; + } + bool Flag(string name) => Array.IndexOf(args, name) >= 0; + + string Required(string variable, string what) => + environment(variable) is { Length: > 0 } value + ? value + : throw new ArgumentException( + $"{variable} is required ({what}). Set it in the environment, or in the `env` block " + + "of your MCP client's server configuration."); + + return new McpHostOptions + { + AzureEndpoint = Required("AZURE_OPENAI_ENDPOINT", "e.g. https://.openai.azure.com/"), + AzureApiKey = Required("AZURE_OPENAI_API_KEY", "the embedding provider key"), + EmbeddingDeployment = environment("AZURE_OPENAI_EMBEDDING_DEPLOYMENT") is { Length: > 0 } d + ? d + : "text-embedding-3-small", + + Neo4jUri = environment("NEO4J_URI") is { Length: > 0 } uri ? uri : "bolt://localhost:7687", + Neo4jUsername = environment("NEO4J_USERNAME") is { Length: > 0 } u ? u : "neo4j", + // No default. A blank password silently becomes an authentication failure at the first + // query, which reads as "the database is empty" from an MCP client. + Neo4jPassword = Required("NEO4J_PASSWORD", "the Neo4j password"), + Neo4jDatabase = environment("NEO4J_DATABASE") is { Length: > 0 } db ? db : "neo4j", + + Transport = ParseTransport(Value("--transport") ?? environment("AGENT_MEMORY_MCP_TRANSPORT")), + HttpUrl = FirstNonEmpty(Value("--url"), environment("AGENT_MEMORY_MCP_URL")) + ?? "http://localhost:5233", + ServerName = FirstNonEmpty(Value("--server-name"), environment("AGENT_MEMORY_MCP_SERVER_NAME")) + ?? "agent-memory", + + // Flag OR environment, never flag-overrides-environment: for a safety switch the union is + // the correct combination. Someone who set the variable and someone who passed the flag + // both asked for read-only, and neither can be cancelled by the other's absence. + ReadOnly = Flag("--read-only") || Boolean(environment("AGENT_MEMORY_MCP_READ_ONLY")), + + EnableGraphQuery = Flag("--enable-graph-query") + || Boolean(environment("AGENT_MEMORY_MCP_ENABLE_GRAPH_QUERY")), + Bootstrap = !Flag("--no-bootstrap") && !Boolean(environment("AGENT_MEMORY_MCP_NO_BOOTSTRAP")), + LogLevel = ParseLogLevel(Value("--log-level") ?? environment("AGENT_MEMORY_MCP_LOG_LEVEL")), + }; + } + + /// The first value that is present and not blank, or null. + /// + /// Blank is treated as absent throughout. An exported-but-empty variable is how a container + /// silently ends up binding to "" and failing at startup with a message about a URL nobody wrote. + /// + private static string? FirstNonEmpty(params string?[] candidates) => + candidates.FirstOrDefault(candidate => !string.IsNullOrWhiteSpace(candidate)); + + private static bool Boolean(string? value) => + value is not null + && (value.Equals("1", StringComparison.Ordinal) + || value.Equals("true", StringComparison.OrdinalIgnoreCase) + || value.Equals("yes", StringComparison.OrdinalIgnoreCase)); + + private static McpHostTransport ParseTransport(string? value) => + value?.ToLowerInvariant() switch + { + null or "" or "stdio" => McpHostTransport.Stdio, + "http" => McpHostTransport.Http, + _ => throw new ArgumentException("--transport must be one of: stdio, http."), + }; + + private static LogLevel ParseLogLevel(string? value) => + value is null or "" ? LogLevel.Information + : Enum.TryParse(value, ignoreCase: true, out var level) ? level + : throw new ArgumentException( + "--log-level must be one of: trace, debug, information, warning, error, critical, none."); + + internal static string HelpText => string.Format( + CultureInfo.InvariantCulture, + """ + agent-memory-mcp - MCP server for AgentMemory for .NET + + agent-memory-mcp [--transport stdio|http] [--url http://localhost:5233] + [--read-only] [--enable-graph-query] [--no-bootstrap] + [--server-name agent-memory] [--log-level information] + + Required environment: + AZURE_OPENAI_ENDPOINT https://.openai.azure.com/ + AZURE_OPENAI_API_KEY embedding provider key + NEO4J_PASSWORD Neo4j password + + Optional environment (defaults shown): + AZURE_OPENAI_EMBEDDING_DEPLOYMENT text-embedding-3-small + NEO4J_URI bolt://localhost:7687 + NEO4J_USERNAME neo4j + NEO4J_DATABASE neo4j + AGENT_MEMORY_MCP_TRANSPORT stdio + AGENT_MEMORY_MCP_URL http://localhost:5233 + AGENT_MEMORY_MCP_READ_ONLY unset + AGENT_MEMORY_MCP_ENABLE_GRAPH_QUERY unset + AGENT_MEMORY_MCP_NO_BOOTSTRAP unset + AGENT_MEMORY_MCP_LOG_LEVEL information + + --read-only removes every tool that writes from the server's tool list entirely, rather than + refusing them when called: a tool a client can see is one a model will try. {0} of the {1} + tools remain. + + --enable-graph-query exposes graph_query, which runs arbitrary Cypher. It is off by default and + is withheld under --read-only regardless, because a mode guaranteeing "nothing changes" must + not rest on a query parser. + + Schema migrations run at startup unless --no-bootstrap. A missing vector index returns no rows + rather than an error, so a server started without them looks healthy and answers nothing. + + Logs go to stderr on both transports; stdout is the JSON-RPC stream on stdio. + """, + McpToolAccess.ReadTools.Count, + McpToolAccess.ReadTools.Count + McpToolAccess.WriteTools.Count); +} + +/// The version this build reports in the MCP handshake. +/// +/// Read from the assembly rather than written as a literal: a hand-maintained version string in a +/// handshake is one that stops matching the package it was installed from, and the handshake is where +/// a client reports what it is talking to. +/// +internal static class ThisAssembly +{ + internal static string Version { get; } = + typeof(ThisAssembly).Assembly.GetName().Version?.ToString(3) ?? "0.0.0"; +} diff --git a/tools/AgentMemory.McpHost/McpHostProgram.cs b/tools/AgentMemory.McpHost/McpHostProgram.cs new file mode 100644 index 00000000..2566da1d --- /dev/null +++ b/tools/AgentMemory.McpHost/McpHostProgram.cs @@ -0,0 +1,150 @@ +using AgentMemory.Abstractions.Services; +using AgentMemory.Core; +using AgentMemory.Core.Stubs; +using AgentMemory.McpServer; +using AgentMemory.Neo4j.Infrastructure; +using Azure; +using Azure.AI.OpenAI; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.AI; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Logging; + +namespace AgentMemory.McpHost; + +/// +/// The turnkey MCP server: 25 memory tools over stdio or HTTP, configured entirely by environment. +/// +/// +/// +/// The tools already existed and there was no way to run them without writing a .NET host first +/// — which meant the audience was people who would build one anyway, and everyone else could not try +/// the project at all. This is that host, packaged as a global tool so trying it is +/// dotnet tool install -g and one command. +/// +/// +/// stdout belongs to the protocol on the stdio transport. Every diagnostic here goes to stderr, +/// including startup failures: one stray Console.WriteLine corrupts the JSON-RPC stream and +/// presents as an unreadable client-side parse error rather than as the log line it is. +/// +/// +internal static class McpHostProgram +{ + internal static async Task RunAsync(string[] args) + { + if (args.Any(argument => argument is "--help" or "-h" or "/?")) + { + Console.Error.WriteLine(McpHostOptions.HelpText); + return 0; + } + + McpHostOptions options; + try + { + options = McpHostOptions.Parse(args, Environment.GetEnvironmentVariable); + } + catch (ArgumentException ex) + { + Console.Error.WriteLine($"agent-memory-mcp: {ex.Message}"); + Console.Error.WriteLine(); + Console.Error.WriteLine(McpHostOptions.HelpText); + return 2; + } + + var builder = WebApplication.CreateSlimBuilder(args); + + // stdout is the protocol on stdio, and there is no reason for a server's own logs to differ by + // transport, so both go to stderr. + builder.Logging.ClearProviders(); + builder.Logging.AddConsole(console => console.LogToStandardErrorThreshold = LogLevel.Trace); + builder.Logging.SetMinimumLevel(options.LogLevel); + + builder.Services.AddNeo4jAgentMemory(neo4j => + { + neo4j.Uri = options.Neo4jUri; + neo4j.Username = options.Neo4jUsername; + neo4j.Password = options.Neo4jPassword; + neo4j.Database = options.Neo4jDatabase; + }); + builder.Services.AddAgentMemoryCore(_ => { }); + builder.Services.AddSingleton(); + builder.Services.AddSingleton(); + builder.Services.AddSingleton>>( + new AzureOpenAIClient(new Uri(options.AzureEndpoint), new AzureKeyCredential(options.AzureApiKey)) + .GetEmbeddingClient(options.EmbeddingDeployment) + .AsIEmbeddingGenerator()); + + var mcp = builder.Services.AddMcpServer(); + if (options.Transport == McpHostTransport.Http) mcp.WithHttpTransport(); + else mcp.WithStdioServerTransport(); + + mcp.AddAgentMemoryMcpTools(mcpOptions => + { + mcpOptions.ServerName = options.ServerName; + mcpOptions.ServerVersion = ThisAssembly.Version; + mcpOptions.EnableGraphQuery = options.EnableGraphQuery; + mcpOptions.ReadOnly = options.ReadOnly; + }) + .AddAgentMemoryMcpPrompts() + .AddAgentMemoryMcpResources(); + + var app = builder.Build(); + + if (options.Bootstrap && !await TryBootstrapAsync(app.Services).ConfigureAwait(false)) + return 1; + + Console.Error.WriteLine( + $"agent-memory-mcp {ThisAssembly.Version}: {options.Transport.ToString().ToLowerInvariant()} transport, " + + $"{(options.ReadOnly ? "READ-ONLY" : "read-write")}, " + + $"graph_query {(options.EnableGraphQuery ? "enabled" : "disabled")}, " + + $"neo4j {options.Neo4jUri}/{options.Neo4jDatabase}."); + + if (options.Transport == McpHostTransport.Http) + { + app.MapMcp(); + app.Urls.Clear(); + app.Urls.Add(options.HttpUrl); + } + + await app.RunAsync().ConfigureAwait(false); + return 0; + } + + /// + /// Applies the schema migrations before serving, and reports a failure as a failure. + /// + /// + /// + /// Every tool needs the indexes and constraints. Without this the first honest experience of the + /// project is a working server whose searches return nothing — a missing vector index produces + /// empty results, not an error, so it reads as "the memory is empty" rather than "the schema was + /// never created". + /// + /// + /// Failing here exits non-zero rather than degrading: an operator who asked for bootstrap and did + /// not get it must not be left with a server that looks healthy. The message says how to skip it, + /// because a read-only database user legitimately cannot create indexes. + /// + /// + private static async Task TryBootstrapAsync(IServiceProvider services) + { + await using var scope = services.CreateAsyncScope(); + try + { + await scope.ServiceProvider.GetRequiredService() + .BootstrapAsync().ConfigureAwait(false); + return true; + } + catch (Exception ex) + { + Console.Error.WriteLine( + $"agent-memory-mcp: schema bootstrap failed: {ex.Message}\n" + + "The server would start but every search would return empty results, because a " + + "missing vector index yields no rows rather than an error. Fix the connection or " + + "the database user's privileges, or pass --no-bootstrap if the schema is already " + + "in place and this user cannot create indexes."); + return false; + } + } +} diff --git a/tools/AgentMemory.McpHost/Program.cs b/tools/AgentMemory.McpHost/Program.cs new file mode 100644 index 00000000..b09f4fbf --- /dev/null +++ b/tools/AgentMemory.McpHost/Program.cs @@ -0,0 +1,5 @@ +using AgentMemory.McpHost; + +// Everything lives in McpHostProgram so the option parsing beside it can be unit-tested; a top-level +// program's members are not reachable from a test project. +return await McpHostProgram.RunAsync(args).ConfigureAwait(false); diff --git a/tools/AgentMemory.McpHost/README.md b/tools/AgentMemory.McpHost/README.md new file mode 100644 index 00000000..65531305 --- /dev/null +++ b/tools/AgentMemory.McpHost/README.md @@ -0,0 +1,111 @@ +# agent-memory-mcp + +A turnkey MCP server for **AgentMemory for .NET**. It exposes the 25 memory tools over stdio or HTTP, +backed by Neo4j, configured entirely by environment variables. + +Before this, the tools shipped with no way to run them without writing a .NET host first — so the only +people who could try them were people who would have built one anyway. + +## Install + +```bash +dotnet tool install -g AgentMemory.McpHost +``` + +You need a Neo4j 5.26 instance and an Azure OpenAI embedding deployment. If you have neither, the +compose file below starts the database and the server together. + +## Run + +```bash +export AZURE_OPENAI_ENDPOINT=https://.openai.azure.com/ +export AZURE_OPENAI_API_KEY= +export NEO4J_PASSWORD= + +agent-memory-mcp # stdio, for a desktop MCP client +agent-memory-mcp --transport http # http://localhost:5233 +agent-memory-mcp --read-only # only the 9 tools that read +``` + +Schema migrations run at startup. That matters more than it sounds: **a missing vector index returns +no rows rather than an error**, so a server started without them looks perfectly healthy and answers +nothing. Pass `--no-bootstrap` only when the schema already exists and the database user cannot create +indexes. + +## MCP client configuration + +```json +{ + "mcpServers": { + "agent-memory": { + "command": "agent-memory-mcp", + "env": { + "AZURE_OPENAI_ENDPOINT": "https://.openai.azure.com/", + "AZURE_OPENAI_API_KEY": "", + "NEO4J_PASSWORD": "" + } + } + } +} +``` + +Logs go to **stderr** on both transports; on stdio, stdout is the JSON-RPC stream and a single stray +line on it corrupts the session. + +## Read-only mode + +`--read-only` (or `AGENT_MEMORY_MCP_READ_ONLY=true`) removes every tool that writes from the server's +tool list **entirely**, rather than refusing them when called. A tool a client can see is a tool a +model will try, and an error return teaches it nothing about what the server is for. + +Nine tools remain: `memory_search`, `memory_get_context`, `memory_get_conversation`, +`memory_list_sessions`, `memory_get_entity`, `memory_get_entity_provenance`, `memory_get_observations`, +`memory_export_graph`, `memory_find_duplicates`. + +`graph_query` is withheld under `--read-only` even though it is nominally a read. It takes arbitrary +Cypher, and a mode whose entire value is "this cannot change anything" should not rest on a query +parser. It is off by default in any case; `--enable-graph-query` turns it on for a read-write server. + +The classification lives in `McpToolAccess`, where **writes are the enumerated set** — so a tool added +later and left unclassified is withheld rather than silently exposed, and a guard test fails if any +tool escapes classification altogether. + +## Docker + +```bash +AZURE_OPENAI_ENDPOINT=... AZURE_OPENAI_API_KEY=... \ + docker compose -f tools/AgentMemory.McpHost/docker-compose.yml up +``` + +Neo4j 5.26 plus the server, from nothing, on `http://localhost:5233`. The compose file waits for the +database to answer before starting the server, because the server bootstraps the schema and exits +non-zero if it cannot — without the healthcheck, a cold volume fails the first run and succeeds the +second, which is the worst possible first impression. + +The image defaults to HTTP and binds `0.0.0.0`: inside a container, `localhost` is reachable only from +inside it, which is the most common way a correctly built image looks broken from the host. + +## Configuration + +| Variable | Default | | +|---|---|---| +| `AZURE_OPENAI_ENDPOINT` | — | **required** | +| `AZURE_OPENAI_API_KEY` | — | **required** | +| `NEO4J_PASSWORD` | — | **required** | +| `AZURE_OPENAI_EMBEDDING_DEPLOYMENT` | `text-embedding-3-small` | | +| `NEO4J_URI` | `bolt://localhost:7687` | | +| `NEO4J_USERNAME` | `neo4j` | | +| `NEO4J_DATABASE` | `neo4j` | | +| `AGENT_MEMORY_MCP_TRANSPORT` | `stdio` | `--transport` | +| `AGENT_MEMORY_MCP_URL` | `http://localhost:5233` | `--url` | +| `AGENT_MEMORY_MCP_SERVER_NAME` | `agent-memory` | `--server-name` | +| `AGENT_MEMORY_MCP_READ_ONLY` | unset | `--read-only` | +| `AGENT_MEMORY_MCP_ENABLE_GRAPH_QUERY` | unset | `--enable-graph-query` | +| `AGENT_MEMORY_MCP_NO_BOOTSTRAP` | unset | `--no-bootstrap` | +| `AGENT_MEMORY_MCP_LOG_LEVEL` | `information` | `--log-level` | + +`NEO4J_PASSWORD` has no default on purpose: a blank password becomes an authentication failure at the +first query, which from an MCP client is indistinguishable from an empty database. + +An unrecognised flag is an error rather than being ignored — a typo in `--read-only` would otherwise +start a fully writable server that the operator believes is read-only. diff --git a/tools/AgentMemory.McpHost/docker-compose.yml b/tools/AgentMemory.McpHost/docker-compose.yml new file mode 100644 index 00000000..cf15b893 --- /dev/null +++ b/tools/AgentMemory.McpHost/docker-compose.yml @@ -0,0 +1,49 @@ +# A complete, runnable stack: Neo4j plus the MCP server, from nothing. +# +# AZURE_OPENAI_ENDPOINT=... AZURE_OPENAI_API_KEY=... \ +# docker compose -f tools/AgentMemory.McpHost/docker-compose.yml up +# +# Then point an MCP client at http://localhost:5233. +services: + neo4j: + image: neo4j:5.26 + environment: + # Matches the version the test suite and every recorded measurement run against. Vector index + # behaviour differs between 5.x minors, so the pin is deliberate rather than incidental. + NEO4J_AUTH: neo4j/${NEO4J_PASSWORD:-agentmemory} + NEO4J_PLUGINS: '["apoc"]' + ports: + - "7474:7474" + - "7687:7687" + volumes: + - neo4j-data:/data + healthcheck: + # The server bootstraps the schema at startup and exits non-zero if it cannot, so it must not + # start before the database can answer. Without this the first run fails on a cold volume and + # succeeds on the second attempt, which is the most confusing possible first impression. + test: ["CMD-SHELL", "wget -qO- http://localhost:7474 || exit 1"] + interval: 5s + timeout: 5s + retries: 30 + + agent-memory-mcp: + build: + context: ../.. + dockerfile: tools/AgentMemory.McpHost/Dockerfile + depends_on: + neo4j: + condition: service_healthy + environment: + NEO4J_URI: bolt://neo4j:7687 + NEO4J_USERNAME: neo4j + NEO4J_PASSWORD: ${NEO4J_PASSWORD:-agentmemory} + AZURE_OPENAI_ENDPOINT: ${AZURE_OPENAI_ENDPOINT:?set AZURE_OPENAI_ENDPOINT} + AZURE_OPENAI_API_KEY: ${AZURE_OPENAI_API_KEY:?set AZURE_OPENAI_API_KEY} + AZURE_OPENAI_EMBEDDING_DEPLOYMENT: ${AZURE_OPENAI_EMBEDDING_DEPLOYMENT:-text-embedding-3-small} + # Uncomment to expose only the 9 tools that read. + # AGENT_MEMORY_MCP_READ_ONLY: "true" + ports: + - "5233:5233" + +volumes: + neo4j-data: