From 6e4c89944d35d52cae5b22182ca01dd52f2c2417 Mon Sep 17 00:00:00 2001 From: "Owen.Morgan-Jones" Date: Thu, 19 Mar 2026 12:19:09 +0000 Subject: [PATCH 1/4] Makes feature flag controllable at runtime --- .../FeatureFlagRepositoryIntegrationTests.cs | 130 ++++++++++++++++ .../ConversationMessageHandlerTests.cs | 18 +++ .../LlmToggleHandlerTests.cs | 147 ++++++++++++++++++ Bottomly/Models/FeatureFlag.cs | 10 ++ Bottomly/Program.cs | 40 +++-- .../Repositories/FeatureFlagRepository.cs | 30 ++++ .../Repositories/IFeatureFlagRepository.cs | 8 + .../ConversationMessageHandler.cs | 7 + .../ImageSearchHandler.cs | 5 +- .../MessageEventHandlers/LlmToggleHandler.cs | 52 +++++++ Bottomly/appsettings.json | 3 +- bottomly.net.slnx | 1 + 12 files changed, 423 insertions(+), 28 deletions(-) create mode 100644 Bottomly.Tests/Repositories/Integration/FeatureFlagRepositoryIntegrationTests.cs create mode 100644 Bottomly.Tests/Slack/MessageEventHandlers/LlmToggleHandlerTests.cs create mode 100644 Bottomly/Models/FeatureFlag.cs create mode 100644 Bottomly/Repositories/FeatureFlagRepository.cs create mode 100644 Bottomly/Repositories/IFeatureFlagRepository.cs create mode 100644 Bottomly/Slack/MessageEventHandlers/LlmToggleHandler.cs diff --git a/Bottomly.Tests/Repositories/Integration/FeatureFlagRepositoryIntegrationTests.cs b/Bottomly.Tests/Repositories/Integration/FeatureFlagRepositoryIntegrationTests.cs new file mode 100644 index 0000000..9cb47a4 --- /dev/null +++ b/Bottomly.Tests/Repositories/Integration/FeatureFlagRepositoryIntegrationTests.cs @@ -0,0 +1,130 @@ +using Bottomly.Repositories; +using Bottomly.Tests.Infrastructure; +using MongoDB.Driver; +using Shouldly; + +namespace Bottomly.Tests.Repositories.Integration; + +[Collection("MongoDB")] +public class FeatureFlagRepositoryIntegrationTests(MongoDbFixture fixture) : IAsyncLifetime +{ + private IMongoDatabase _db = null!; + private FeatureFlagRepository _sut = null!; + + public Task InitializeAsync() + { + _db = fixture.GetDatabase($"flags_test_{Guid.NewGuid():N}"); + _sut = new FeatureFlagRepository(_db); + return Task.CompletedTask; + } + + public async Task DisposeAsync() => + await fixture.Client.DropDatabaseAsync(_db.DatabaseNamespace.DatabaseName); + + // ─── GetAsync ────────────────────────────────────────────────────────────── + + [Fact] + public async Task GetAsync_WhenFlagDoesNotExist_ReturnsFalse() + { + var result = await _sut.GetAsync("EnableLlm"); + + result.ShouldBeFalse(); + } + + [Fact] + public async Task GetAsync_WhenFlagIsTrue_ReturnsTrue() + { + await _sut.SetAsync("EnableLlm", true); + + var result = await _sut.GetAsync("EnableLlm"); + + result.ShouldBeTrue(); + } + + [Fact] + public async Task GetAsync_WhenFlagIsFalse_ReturnsFalse() + { + await _sut.SetAsync("EnableLlm", false); + + var result = await _sut.GetAsync("EnableLlm"); + + result.ShouldBeFalse(); + } + + // ─── SetAsync ────────────────────────────────────────────────────────────── + + [Fact] + public async Task SetAsync_CreatesDocumentWhenItDoesNotExist() + { + await _sut.SetAsync("EnableLlm", true); + + var result = await _sut.GetAsync("EnableLlm"); + result.ShouldBeTrue(); + } + + [Fact] + public async Task SetAsync_UpdatesExistingDocument() + { + await _sut.SetAsync("EnableLlm", true); + await _sut.SetAsync("EnableLlm", false); + + var result = await _sut.GetAsync("EnableLlm"); + result.ShouldBeFalse(); + } + + [Fact] + public async Task SetAsync_ToggleOnThenOff_ReturnsCorrectValue() + { + await _sut.SetAsync("EnableLlm", false); + await _sut.SetAsync("EnableLlm", true); + await _sut.SetAsync("EnableLlm", false); + + var result = await _sut.GetAsync("EnableLlm"); + result.ShouldBeFalse(); + } + + // ─── SeedAsync ───────────────────────────────────────────────────────────── + + [Fact] + public async Task SeedAsync_WhenFlagDoesNotExist_CreatesWithDefaultValue() + { + await _sut.SeedAsync("EnableLlm", true); + + var result = await _sut.GetAsync("EnableLlm"); + result.ShouldBeTrue(); + } + + [Fact] + public async Task SeedAsync_WhenFlagAlreadyExists_DoesNotOverwrite() + { + await _sut.SetAsync("EnableLlm", true); + + await _sut.SeedAsync("EnableLlm", false); + + var result = await _sut.GetAsync("EnableLlm"); + result.ShouldBeTrue(); + } + + [Fact] + public async Task SeedAsync_CalledMultipleTimes_Idempotent() + { + await _sut.SeedAsync("EnableLlm", false); + await _sut.SeedAsync("EnableLlm", true); + await _sut.SeedAsync("EnableLlm", true); + + var result = await _sut.GetAsync("EnableLlm"); + result.ShouldBeFalse(); + } + + // ─── Multiple flags ──────────────────────────────────────────────────────── + + [Fact] + public async Task SetAsync_DifferentFlagIds_AreStoredIndependently() + { + await _sut.SetAsync("EnableLlm", true); + await _sut.SetAsync("AnotherFlag", false); + + (await _sut.GetAsync("EnableLlm")).ShouldBeTrue(); + (await _sut.GetAsync("AnotherFlag")).ShouldBeFalse(); + } +} diff --git a/Bottomly.Tests/Slack/MessageEventHandlers/ConversationMessageHandling/ConversationMessageHandlerTests.cs b/Bottomly.Tests/Slack/MessageEventHandlers/ConversationMessageHandling/ConversationMessageHandlerTests.cs index 4fe63eb..d1af414 100644 --- a/Bottomly.Tests/Slack/MessageEventHandlers/ConversationMessageHandling/ConversationMessageHandlerTests.cs +++ b/Bottomly.Tests/Slack/MessageEventHandlers/ConversationMessageHandling/ConversationMessageHandlerTests.cs @@ -19,17 +19,20 @@ public class ConversationMessageHandlerTests private readonly Mock _mockApiClient = new(); private readonly Mock _mockConversations = new(); private readonly Mock _mockMemberRepo = new(); + private readonly Mock _mockFeatureFlagRepo = new(); private readonly ConversationMessageHandler _handler; public ConversationMessageHandlerTests() { _mockApiClient.Setup(a => a.Conversations).Returns(_mockConversations.Object); + _mockFeatureFlagRepo.Setup(r => r.GetAsync("EnableLlm")).ReturnsAsync(true); _handler = new ConversationMessageHandler( _mockLlmBroker.Object, _mockSlackBroker.Object, _mockApiClient.Object, _mockMemberRepo.Object, + _mockFeatureFlagRepo.Object, NullLogger.Instance); } @@ -123,6 +126,21 @@ public async Task HandleAsync_ErrorLlmResponseInThread_SendsReplyToThread() _mockSlackBroker.Verify(b => b.SendMessageAsync(It.IsAny(), "C1", "thread_ts1"), Times.Once()); } + [Fact] + public async Task HandleAsync_LlmFlagDisabled_SkipsLlmAndSendsNothing() + { + _mockFeatureFlagRepo.Setup(r => r.GetAsync("EnableLlm")).ReturnsAsync(false); + + await _handler.HandleAsync(CreateMessage("bottomly what is 2+2?", "U1", "C1")); + + _mockLlmBroker.Verify( + b => b.Respond(It.IsAny(), It.IsAny()), + Times.Never()); + _mockSlackBroker.Verify( + b => b.SendMessageAsync(It.IsAny(), It.IsAny(), It.IsAny()), + Times.Never()); + } + private void SetupConversationHistory(string channel, List messages) => _mockConversations .Setup(c => c.History(channel, It.IsAny(), It.IsAny(), It.IsAny(), diff --git a/Bottomly.Tests/Slack/MessageEventHandlers/LlmToggleHandlerTests.cs b/Bottomly.Tests/Slack/MessageEventHandlers/LlmToggleHandlerTests.cs new file mode 100644 index 0000000..f71a4db --- /dev/null +++ b/Bottomly.Tests/Slack/MessageEventHandlers/LlmToggleHandlerTests.cs @@ -0,0 +1,147 @@ +using Bottomly.Configuration; +using Bottomly.Models; +using Bottomly.Repositories; +using Bottomly.Slack; +using Bottomly.Slack.MessageEventHandlers; +using Bottomly.Tests.Helpers; +using Microsoft.Extensions.Logging.Abstractions; +using Moq; +using Shouldly; +using SlackNet.Events; + +namespace Bottomly.Tests.Slack.MessageEventHandlers; + +public class LlmToggleHandlerTests +{ + private readonly Mock _mockFlagRepo = new(); + private readonly Mock _mockMemberRepo = new(); + private readonly Mock _mockBroker = new(); + private readonly LlmToggleHandler _handler; + + public LlmToggleHandlerTests() + { + _handler = new LlmToggleHandler( + _mockFlagRepo.Object, + _mockMemberRepo.Object, + _mockBroker.Object, + TestHelpers.CreateOptions(), + NullLogger.Instance); + } + + private static MessageEvent CreateMessage(string text, string user = "U_OWEN") => + new() { Text = text, User = user, Channel = "C1", Ts = "ts1" }; + + private void SetupMemberForSlackId(string slackId, string username) => + _mockMemberRepo + .Setup(r => r.GetBySlackIdAsync(slackId)) + .ReturnsAsync(new Member { SlackId = slackId, Username = username }); + + // ─── CanHandle ───────────────────────────────────────────────────────────── + + [Theory] + [InlineData("_llm on")] + [InlineData("_llm off")] + [InlineData("_llm")] + [InlineData("_llm unknown")] + public void CanHandle_LlmCommand_ReturnsTrue(string text) => + _handler.CanHandle(CreateMessage(text)).ShouldBeTrue(); + + [Theory] + [InlineData("_karma alice")] + [InlineData("bottomly what's up?")] + [InlineData("_search something")] + [InlineData("")] + public void CanHandle_NonLlmCommand_ReturnsFalse(string text) => + _handler.CanHandle(CreateMessage(text)).ShouldBeFalse(); + + // ─── Permission check ────────────────────────────────────────────────────── + + [Fact] + public async Task HandleAsync_NonOwenUser_RepliesWithPermissionDenied() + { + SetupMemberForSlackId("U_OTHER", "alice"); + + await _handler.HandleAsync(CreateMessage("_llm on", "U_OTHER")); + + _mockBroker.Verify( + b => b.SendMessageAsync(It.Is(s => s.Contains("permission")), "C1", It.IsAny()), + Times.Once()); + _mockFlagRepo.Verify(r => r.SetAsync(It.IsAny(), It.IsAny()), Times.Never()); + } + + [Fact] + public async Task HandleAsync_UnknownUser_RepliesWithPermissionDenied() + { + _mockMemberRepo.Setup(r => r.GetBySlackIdAsync("U_UNKNOWN")).ReturnsAsync((Member?)null); + + await _handler.HandleAsync(CreateMessage("_llm on", "U_UNKNOWN")); + + _mockBroker.Verify( + b => b.SendMessageAsync(It.Is(s => s.Contains("permission")), "C1", It.IsAny()), + Times.Once()); + _mockFlagRepo.Verify(r => r.SetAsync(It.IsAny(), It.IsAny()), Times.Never()); + } + + // ─── !llm on ─────────────────────────────────────────────────────────────── + + [Fact] + public async Task HandleAsync_LlmOn_OwenUser_EnablesFlag() + { + SetupMemberForSlackId("U_OWEN", "owen"); + + await _handler.HandleAsync(CreateMessage("_llm on", "U_OWEN")); + + _mockFlagRepo.Verify(r => r.SetAsync("EnableLlm", true), Times.Once()); + } + + [Fact] + public async Task HandleAsync_LlmOn_OwenUser_ConfirmsEnabled() + { + SetupMemberForSlackId("U_OWEN", "owen"); + + await _handler.HandleAsync(CreateMessage("_llm on", "U_OWEN")); + + _mockBroker.Verify( + b => b.SendMessageAsync(It.Is(s => s.Contains("enabled", StringComparison.OrdinalIgnoreCase)), "C1", It.IsAny()), + Times.Once()); + } + + // ─── !llm off ────────────────────────────────────────────────────────────── + + [Fact] + public async Task HandleAsync_LlmOff_OwenUser_DisablesFlag() + { + SetupMemberForSlackId("U_OWEN", "owen"); + + await _handler.HandleAsync(CreateMessage("_llm off", "U_OWEN")); + + _mockFlagRepo.Verify(r => r.SetAsync("EnableLlm", false), Times.Once()); + } + + [Fact] + public async Task HandleAsync_LlmOff_OwenUser_ConfirmsDisabled() + { + SetupMemberForSlackId("U_OWEN", "owen"); + + await _handler.HandleAsync(CreateMessage("_llm off", "U_OWEN")); + + _mockBroker.Verify( + b => b.SendMessageAsync(It.Is(s => s.Contains("disabled", StringComparison.OrdinalIgnoreCase)), "C1", It.IsAny()), + Times.Once()); + } + + // ─── Unknown arg ─────────────────────────────────────────────────────────── + + [Fact] + public async Task HandleAsync_UnknownArg_OwenUser_RepliesWithUsageError() + { + SetupMemberForSlackId("U_OWEN", "owen"); + + await _handler.HandleAsync(CreateMessage("_llm blah", "U_OWEN")); + + _mockBroker.Verify( + b => b.SendMessageAsync(It.Is(s => s.Contains("blah")), "C1", It.IsAny()), + Times.Once()); + _mockFlagRepo.Verify(r => r.SetAsync(It.IsAny(), It.IsAny()), Times.Never()); + } +} diff --git a/Bottomly/Models/FeatureFlag.cs b/Bottomly/Models/FeatureFlag.cs new file mode 100644 index 0000000..6c578a4 --- /dev/null +++ b/Bottomly/Models/FeatureFlag.cs @@ -0,0 +1,10 @@ +using MongoDB.Bson.Serialization.Attributes; + +namespace Bottomly.Models; + +public class FeatureFlag +{ + [BsonId][BsonElement("_id")] public string Id { get; set; } = string.Empty; + + [BsonElement("enabled")] public bool Enabled { get; set; } +} diff --git a/Bottomly/Program.cs b/Bottomly/Program.cs index b4c2f9c..e1ab1a7 100644 --- a/Bottomly/Program.cs +++ b/Bottomly/Program.cs @@ -57,6 +57,7 @@ // Repositories builder.Services.AddSingleton(); builder.Services.AddSingleton(); +builder.Services.AddSingleton(); // Commands builder.RegisterCommands(Assembly.GetExecutingAssembly()); @@ -77,9 +78,7 @@ .RegisterEventHandler()); // Event handlers (registered for IEventHandler collection, excluding Help which is separate) -builder.RegisterEventHandlers(Assembly.GetExecutingAssembly(), opts.Value.EnableLlm - ? [] - : [typeof(ConversationMessageHandler)]); +builder.RegisterEventHandlers(Assembly.GetExecutingAssembly(), []); // Help handler (also registered as singleton for direct injection into SlackWorker) builder.Services.AddSingleton(); @@ -99,29 +98,23 @@ builder.Services.AddHostedService(sp => sp.GetRequiredService()); // LLM Support -if (opts.Value.EnableLlm) -{ - builder.AddOllamaApiClient("bottomlymodel") - .AddChatClient(); - - // builder.Services.AddChatClient( - // new OllamaApiClient(new Uri("http://localhost:11434"), "qwen3.5:4b")); +builder.AddOllamaApiClient("bottomlymodel") + .AddChatClient(); - // The built-in resilience settings are super aggressive, with a 10s timeout. - // Running locally Qwen3 takes ~2m to respond to simple queries, so we need to override the defaults. +// The built-in resilience settings are super aggressive, with a 10s timeout. +// Running locally Qwen3 takes ~2m to respond to simple queries, so we need to override the defaults. #pragma warning disable EXTEXP0001 - builder.Services.AddHttpClient("bottomlymodel_httpClient") - .RemoveAllResilienceHandlers() +builder.Services.AddHttpClient("bottomlymodel_httpClient") + .RemoveAllResilienceHandlers() #pragma warning restore EXTEXP0001 - .AddStandardResilienceHandler(options => - { - options.TotalRequestTimeout.Timeout = TimeSpan.FromMinutes(10); - options.AttemptTimeout.Timeout = TimeSpan.FromMinutes(4); - options.CircuitBreaker.SamplingDuration = TimeSpan.FromMinutes(8); - }); + .AddStandardResilienceHandler(options => + { + options.TotalRequestTimeout.Timeout = TimeSpan.FromMinutes(10); + options.AttemptTimeout.Timeout = TimeSpan.FromMinutes(4); + options.CircuitBreaker.SamplingDuration = TimeSpan.FromMinutes(8); + }); - builder.Services.AddTransient(); -} +builder.Services.AddTransient(); // Seeding builder.Services.AddSingleton(); @@ -129,6 +122,9 @@ var app = builder.Build(); +var featureFlagRepository = app.Services.GetRequiredService(); +await featureFlagRepository.SeedAsync("EnableLlm", opts.Value.EnableLlm); + var populator = app.Services.GetRequiredService(); await populator.PopulateMembers(); diff --git a/Bottomly/Repositories/FeatureFlagRepository.cs b/Bottomly/Repositories/FeatureFlagRepository.cs new file mode 100644 index 0000000..0dbfc47 --- /dev/null +++ b/Bottomly/Repositories/FeatureFlagRepository.cs @@ -0,0 +1,30 @@ +using Bottomly.Models; +using MongoDB.Driver; + +namespace Bottomly.Repositories; + +public class FeatureFlagRepository(IMongoDatabase database) : IFeatureFlagRepository +{ + private readonly IMongoCollection _collection = + database.GetCollection("feature_flags"); + + public async Task GetAsync(string flagId) + { + var flag = await _collection.Find(f => f.Id == flagId).FirstOrDefaultAsync(); + return flag?.Enabled ?? false; + } + + public async Task SetAsync(string flagId, bool enabled) + { + var filter = Builders.Filter.Eq(f => f.Id, flagId); + var update = Builders.Update.Set(f => f.Enabled, enabled); + await _collection.UpdateOneAsync(filter, update, new UpdateOptions { IsUpsert = true }); + } + + public async Task SeedAsync(string flagId, bool defaultValue) + { + var filter = Builders.Filter.Eq(f => f.Id, flagId); + var update = Builders.Update.SetOnInsert(f => f.Enabled, defaultValue); + await _collection.UpdateOneAsync(filter, update, new UpdateOptions { IsUpsert = true }); + } +} diff --git a/Bottomly/Repositories/IFeatureFlagRepository.cs b/Bottomly/Repositories/IFeatureFlagRepository.cs new file mode 100644 index 0000000..805b29f --- /dev/null +++ b/Bottomly/Repositories/IFeatureFlagRepository.cs @@ -0,0 +1,8 @@ +namespace Bottomly.Repositories; + +public interface IFeatureFlagRepository +{ + Task GetAsync(string flagId); + Task SetAsync(string flagId, bool enabled); + Task SeedAsync(string flagId, bool defaultValue); +} diff --git a/Bottomly/Slack/MessageEventHandlers/ConversationMessageHandling/ConversationMessageHandler.cs b/Bottomly/Slack/MessageEventHandlers/ConversationMessageHandling/ConversationMessageHandler.cs index 15bde8b..899a534 100644 --- a/Bottomly/Slack/MessageEventHandlers/ConversationMessageHandling/ConversationMessageHandler.cs +++ b/Bottomly/Slack/MessageEventHandlers/ConversationMessageHandling/ConversationMessageHandler.cs @@ -11,6 +11,7 @@ public class ConversationMessageHandler( ISlackMessageBroker slackBroker, ISlackApiClient apiClient, IMemberRepository memberRepository, + IFeatureFlagRepository featureFlagRepository, ILogger logger ) : IMessageEventHandler { @@ -20,6 +21,12 @@ public async Task HandleAsync(MessageEvent message) { logger.LogInformation("Handling conversation message from {User} in {Channel}", message.User, message.Channel); + if (!await featureFlagRepository.GetAsync("EnableLlm")) + { + logger.LogInformation("LLM is disabled; skipping conversation handling."); + return; + } + var history = await apiClient.Conversations.History(message.Channel, limit: 11); var contextUsersSlackIds = history.Messages.Select(m => m.User).Distinct(); diff --git a/Bottomly/Slack/MessageEventHandlers/ImageSearchHandler.cs b/Bottomly/Slack/MessageEventHandlers/ImageSearchHandler.cs index 26bb5a8..81dff67 100644 --- a/Bottomly/Slack/MessageEventHandlers/ImageSearchHandler.cs +++ b/Bottomly/Slack/MessageEventHandlers/ImageSearchHandler.cs @@ -19,10 +19,7 @@ public class ImageSearchHandler( protected override ICommand Command => command; protected override string CommandSymbol => "gi"; - protected override string GetUsage() - { - return CommandTrigger + ""; - } + protected override string GetUsage() => CommandTrigger + ""; protected override async Task InvokeHandlerLogicAsync(MessageEvent message) { diff --git a/Bottomly/Slack/MessageEventHandlers/LlmToggleHandler.cs b/Bottomly/Slack/MessageEventHandlers/LlmToggleHandler.cs new file mode 100644 index 0000000..1d753a2 --- /dev/null +++ b/Bottomly/Slack/MessageEventHandlers/LlmToggleHandler.cs @@ -0,0 +1,52 @@ +using Bottomly.Configuration; +using Bottomly.Repositories; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +using SlackNet.Events; + +namespace Bottomly.Slack.MessageEventHandlers; + +public class LlmToggleHandler( + IFeatureFlagRepository featureFlagRepository, + IMemberRepository memberRepository, + ISlackMessageBroker broker, + IOptions options, + ILogger logger) + : AbstractMessageEventHandler(broker, options, logger) +{ + private const string AuthorisedUser = "owen"; + + public override string Name => "LLM Toggle"; + protected override string CommandSymbol => "llm"; + protected override string GetPurpose() => "Enables or disables the LLM at runtime."; + protected override string GetUsage() => $"{CommandTrigger.TrimEnd()} on|off"; + + public override bool CanHandle(MessageEvent message) => + message.Text?.StartsWith(CommandTrigger.TrimEnd()) == true; + + protected override async Task InvokeHandlerLogicAsync(MessageEvent message) + { + var sender = await memberRepository.GetBySlackIdAsync(message.User); + if (sender?.Username != AuthorisedUser) + { + await SendMessageResponseAsync("You don't have permission to toggle the LLM.", message, true); + return; + } + + var arg = message.Text![CommandTrigger.TrimEnd().Length..].Trim().ToLowerInvariant(); + switch (arg) + { + case "on": + await featureFlagRepository.SetAsync("EnableLlm", true); + await SendMessageResponseAsync("LLM enabled.", message, true); + break; + case "off": + await featureFlagRepository.SetAsync("EnableLlm", false); + await SendMessageResponseAsync("LLM disabled.", message, true); + break; + default: + await SendMessageResponseAsync($"Unknown argument '{arg}'. Use `{GetUsage()}`.", message, true); + break; + } + } +} \ No newline at end of file diff --git a/Bottomly/appsettings.json b/Bottomly/appsettings.json index b5454b5..8f8ce5c 100644 --- a/Bottomly/appsettings.json +++ b/Bottomly/appsettings.json @@ -7,6 +7,5 @@ "MongoDB": "Information" } }, - "ImportMemberSeedData": true, - "EnableLlm": false + "ImportMemberSeedData": true } \ No newline at end of file diff --git a/bottomly.net.slnx b/bottomly.net.slnx index 0086036..d78ed50 100644 --- a/bottomly.net.slnx +++ b/bottomly.net.slnx @@ -1,5 +1,6 @@ + From 9284402ba9815c60fa851a1ed2162def4a4dfc3a Mon Sep 17 00:00:00 2001 From: "Owen.Morgan-Jones" Date: Thu, 19 Mar 2026 12:38:57 +0000 Subject: [PATCH 2/4] Identified an issue with @bottomly messages Because the "@bottomly" would be tokenised by slack, we need to lookup using the member repo. However, this would introduce a LOT of lookups and I always had vague thoughts of caching the member repo anyway. This commit adds memory caching to the member repo which is warmed at app startup. This way all the token replacements should be signficantly faster as all calls for member data should be obviated. --- .../Unit/CachingMemberRepositoryTests.cs | 161 ++++++++++++++++++ .../ConversationMessageHandlerTests.cs | 5 + Bottomly/Program.cs | 8 +- .../Repositories/CachingMemberRepository.cs | 97 +++++++++++ Bottomly/Repositories/IMemberRepository.cs | 1 + Bottomly/Repositories/MemberCachePopulator.cs | 16 ++ Bottomly/Repositories/MemberRepository.cs | 3 + .../ConversationMessageHandler.cs | 11 +- 8 files changed, 300 insertions(+), 2 deletions(-) create mode 100644 Bottomly.Tests/Repositories/Unit/CachingMemberRepositoryTests.cs create mode 100644 Bottomly/Repositories/CachingMemberRepository.cs create mode 100644 Bottomly/Repositories/MemberCachePopulator.cs diff --git a/Bottomly.Tests/Repositories/Unit/CachingMemberRepositoryTests.cs b/Bottomly.Tests/Repositories/Unit/CachingMemberRepositoryTests.cs new file mode 100644 index 0000000..f92aad7 --- /dev/null +++ b/Bottomly.Tests/Repositories/Unit/CachingMemberRepositoryTests.cs @@ -0,0 +1,161 @@ +using Bottomly.Models; +using Bottomly.Repositories; +using Microsoft.Extensions.Caching.Memory; +using Microsoft.Extensions.Options; +using Moq; +using Shouldly; + +namespace Bottomly.Tests.Repositories.Unit; + +public class CachingMemberRepositoryTests +{ + private readonly Mock _mockInner = new(); + private readonly IMemoryCache _cache = new MemoryCache(Options.Create(new MemoryCacheOptions())); + private readonly CachingMemberRepository _repo; + + public CachingMemberRepositoryTests() + { + _repo = new CachingMemberRepository(_mockInner.Object, _cache); + } + + [Fact] + public async Task GetBySlackIdAsync_CacheMiss_CallsInnerAndCachesResult() + { + var member = new Member { SlackId = "U1", Username = "alice" }; + _mockInner.Setup(r => r.GetBySlackIdAsync("U1")).ReturnsAsync(member); + + var result = await _repo.GetBySlackIdAsync("U1"); + + result.ShouldBe(member); + _mockInner.Verify(r => r.GetBySlackIdAsync("U1"), Times.Once); + } + + [Fact] + public async Task GetBySlackIdAsync_CacheHit_DoesNotCallInner() + { + var member = new Member { SlackId = "U1", Username = "alice" }; + _mockInner.Setup(r => r.GetBySlackIdAsync("U1")).ReturnsAsync(member); + + await _repo.GetBySlackIdAsync("U1"); // Populates cache + await _repo.GetBySlackIdAsync("U1"); // Should hit cache + + _mockInner.Verify(r => r.GetBySlackIdAsync("U1"), Times.Once); + } + + [Fact] + public async Task GetByUsernameAsync_CacheMiss_CallsInnerAndCachesResult() + { + var member = new Member { SlackId = "U1", Username = "alice" }; + _mockInner.Setup(r => r.GetByUsernameAsync("alice")).ReturnsAsync(member); + + var result = await _repo.GetByUsernameAsync("alice"); + + result.ShouldBe(member); + _mockInner.Verify(r => r.GetByUsernameAsync("alice"), Times.Once); + } + + [Fact] + public async Task GetByUsernameAsync_CacheHit_DoesNotCallInner() + { + var member = new Member { SlackId = "U1", Username = "alice" }; + _mockInner.Setup(r => r.GetByUsernameAsync("alice")).ReturnsAsync(member); + + await _repo.GetByUsernameAsync("alice"); // Populates cache + await _repo.GetByUsernameAsync("alice"); // Should hit cache + + _mockInner.Verify(r => r.GetByUsernameAsync("alice"), Times.Once); + } + + [Fact] + public async Task GetBySlackIdAsync_AfterGetByUsernameAsync_DoesNotCallInner() + { + var member = new Member { SlackId = "U1", Username = "alice" }; + _mockInner.Setup(r => r.GetByUsernameAsync("alice")).ReturnsAsync(member); + + await _repo.GetByUsernameAsync("alice"); // Caches by both keys + await _repo.GetBySlackIdAsync("U1"); // Should hit cache (cross-key) + + _mockInner.Verify(r => r.GetBySlackIdAsync("U1"), Times.Never); + } + + [Fact] + public async Task GetAllAsync_PopulatesBothCacheKeys() + { + var members = new List + { + new() { SlackId = "U1", Username = "alice" }, + new() { SlackId = "U2", Username = "bob" } + }; + _mockInner.Setup(r => r.GetAllAsync()).ReturnsAsync(members); + + var result = await _repo.GetAllAsync(); + + result.ShouldBe(members); + // Subsequent lookups should hit cache + await _repo.GetBySlackIdAsync("U1"); + await _repo.GetByUsernameAsync("bob"); + _mockInner.Verify(r => r.GetBySlackIdAsync(It.IsAny()), Times.Never); + _mockInner.Verify(r => r.GetByUsernameAsync(It.IsAny()), Times.Never); + } + + [Fact] + public async Task GetBySlackIdsAsync_AllCached_DoesNotCallInner() + { + var members = new List + { + new() { SlackId = "U1", Username = "alice" }, + new() { SlackId = "U2", Username = "bob" } + }; + _mockInner.Setup(r => r.GetAllAsync()).ReturnsAsync(members); + await _repo.GetAllAsync(); // Warm the cache + + var result = await _repo.GetBySlackIdsAsync(["U1", "U2"]); + + result.Count.ShouldBe(2); + _mockInner.Verify(r => r.GetBySlackIdsAsync(It.IsAny>()), Times.Never); + } + + [Fact] + public async Task GetBySlackIdsAsync_PartialCacheMiss_FetchesMissesFromInner() + { + var cachedMember = new Member { SlackId = "U1", Username = "alice" }; + var missedMember = new Member { SlackId = "U2", Username = "bob" }; + _mockInner.Setup(r => r.GetBySlackIdAsync("U1")).ReturnsAsync(cachedMember); + _mockInner.Setup(r => r.GetBySlackIdsAsync(It.Is>(ids => ids.Contains("U2")))) + .ReturnsAsync([missedMember]); + + await _repo.GetBySlackIdAsync("U1"); // Cache U1 only + + var result = await _repo.GetBySlackIdsAsync(["U1", "U2"]); + + result.Count.ShouldBe(2); + _mockInner.Verify(r => r.GetBySlackIdsAsync(It.IsAny>()), Times.Once); + } + + [Fact] + public async Task AddAsync_SingleMember_CachesMember() + { + var member = new Member { SlackId = "U1", Username = "alice" }; + _mockInner.Setup(r => r.AddAsync(member)).Returns(Task.CompletedTask); + + await _repo.AddAsync(member); + await _repo.GetBySlackIdAsync("U1"); + + _mockInner.Verify(r => r.GetBySlackIdAsync(It.IsAny()), Times.Never); + } + + [Fact] + public async Task UpdateInfoAsync_UpdatesCacheWithFreshData() + { + var updated = new Member { SlackId = "U1", Username = "alice", FullName = "Alice Smith" }; + _mockInner.Setup(r => r.UpdateInfoAsync("alice", "Alice Smith", It.IsAny(), It.IsAny(), It.IsAny())) + .Returns(Task.CompletedTask); + _mockInner.Setup(r => r.GetByUsernameAsync("alice")).ReturnsAsync(updated); + + await _repo.UpdateInfoAsync("alice", "Alice Smith", Gender.Unknown, SassLevel.Moderate, string.Empty); + var result = await _repo.GetByUsernameAsync("alice"); + + result!.FullName.ShouldBe("Alice Smith"); + _mockInner.Verify(r => r.GetByUsernameAsync("alice"), Times.Once); // Called once during UpdateInfoAsync + } +} diff --git a/Bottomly.Tests/Slack/MessageEventHandlers/ConversationMessageHandling/ConversationMessageHandlerTests.cs b/Bottomly.Tests/Slack/MessageEventHandlers/ConversationMessageHandling/ConversationMessageHandlerTests.cs index d1af414..9c8147d 100644 --- a/Bottomly.Tests/Slack/MessageEventHandlers/ConversationMessageHandling/ConversationMessageHandlerTests.cs +++ b/Bottomly.Tests/Slack/MessageEventHandlers/ConversationMessageHandling/ConversationMessageHandlerTests.cs @@ -26,6 +26,8 @@ public ConversationMessageHandlerTests() { _mockApiClient.Setup(a => a.Conversations).Returns(_mockConversations.Object); _mockFeatureFlagRepo.Setup(r => r.GetAsync("EnableLlm")).ReturnsAsync(true); + _mockMemberRepo.Setup(r => r.GetByUsernameAsync("bottomly")) + .ReturnsAsync(new Member { Username = "bottomly", SlackId = "UBOTID" }); _handler = new ConversationMessageHandler( _mockLlmBroker.Object, @@ -43,6 +45,8 @@ private static MessageEvent CreateMessage(string text, string user = "U1", strin [InlineData("hey bottomly what do you think?")] [InlineData("bottomly, help me")] [InlineData("I asked bottomly already")] + [InlineData("<@UBOTID> what do you think?")] + [InlineData("<@UBOTID>")] public void CanHandle_MessageContainsBottomly_ReturnsTrue(string text) => _handler.CanHandle(CreateMessage(text)).ShouldBeTrue(); @@ -50,6 +54,7 @@ public void CanHandle_MessageContainsBottomly_ReturnsTrue(string text) => [InlineData("hello there")] [InlineData("_karma alice")] [InlineData("")] + [InlineData("<@UOTHERID> what do you think?")] public void CanHandle_MessageWithoutBottomly_ReturnsFalse(string text) => _handler.CanHandle(CreateMessage(text)).ShouldBeFalse(); diff --git a/Bottomly/Program.cs b/Bottomly/Program.cs index e1ab1a7..b55d7c0 100644 --- a/Bottomly/Program.cs +++ b/Bottomly/Program.cs @@ -9,6 +9,7 @@ using Bottomly.Slack.MessageEventHandlers; using Bottomly.Slack.MessageEventHandlers.ConversationMessageHandling; using Bottomly.Slack.ReactionHandlers; +using Microsoft.Extensions.Caching.Memory; using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; @@ -55,8 +56,12 @@ builder.Services.AddHttpClient(); // Repositories +builder.Services.AddMemoryCache(); builder.Services.AddSingleton(); -builder.Services.AddSingleton(); +builder.Services.AddSingleton(sp => + new CachingMemberRepository( + new MemberRepository(sp.GetRequiredService()), + sp.GetRequiredService())); builder.Services.AddSingleton(); // Commands @@ -95,6 +100,7 @@ builder.Services.AddSingleton(); builder.Services.AddSingleton(); builder.Services.AddSingleton(); +builder.Services.AddHostedService(); builder.Services.AddHostedService(sp => sp.GetRequiredService()); // LLM Support diff --git a/Bottomly/Repositories/CachingMemberRepository.cs b/Bottomly/Repositories/CachingMemberRepository.cs new file mode 100644 index 0000000..fec0737 --- /dev/null +++ b/Bottomly/Repositories/CachingMemberRepository.cs @@ -0,0 +1,97 @@ +using Bottomly.Models; +using Microsoft.Extensions.Caching.Memory; + +namespace Bottomly.Repositories; + +public class CachingMemberRepository(IMemberRepository inner, IMemoryCache cache) : IMemberRepository +{ + private const string SlackKeyPrefix = "member:slack:"; + private const string UsernameKeyPrefix = "member:username:"; + + public async Task> GetAllAsync() + { + var members = await inner.GetAllAsync(); + foreach (var member in members) + CacheMember(member); + return members; + } + + public async Task GetByUsernameAsync(string username) + { + if (cache.TryGetValue(UsernameKeyPrefix + username, out Member? cached)) + return cached; + + var member = await inner.GetByUsernameAsync(username); + if (member is not null) + CacheMember(member); + return member; + } + + public async Task GetBySlackIdAsync(string slackId) + { + if (cache.TryGetValue(SlackKeyPrefix + slackId, out Member? cached)) + return cached; + + var member = await inner.GetBySlackIdAsync(slackId); + if (member is not null) + CacheMember(member); + return member; + } + + public async Task> GetBySlackIdsAsync(IEnumerable slackIds) + { + var idList = slackIds.ToList(); + var result = new List(); + var misses = new List(); + + foreach (var id in idList) + { + if (cache.TryGetValue(SlackKeyPrefix + id, out Member? cached)) + result.Add(cached!); + else + misses.Add(id); + } + + if (misses.Count > 0) + { + var fetched = await inner.GetBySlackIdsAsync(misses); + foreach (var member in fetched) + { + CacheMember(member); + result.Add(member); + } + } + + return result; + } + + public async Task AddAsync(Member member) + { + await inner.AddAsync(member); + CacheMember(member); + } + + public async Task AddAsync(IEnumerable members) + { + var memberList = members.ToList(); + await inner.AddAsync(memberList); + foreach (var member in memberList) + CacheMember(member); + } + + public async Task UpdateInfoAsync(string username, string fullName, Gender gender, SassLevel sassLevel, string miscInfo) + { + await inner.UpdateInfoAsync(username, fullName, gender, sassLevel, miscInfo); + var updated = await inner.GetByUsernameAsync(username); + if (updated is not null) + CacheMember(updated); + else + cache.Remove(UsernameKeyPrefix + username); + } + + private void CacheMember(Member member) + { + cache.Set(SlackKeyPrefix + member.SlackId, member); + cache.Set(UsernameKeyPrefix + member.Username, member); + } +} diff --git a/Bottomly/Repositories/IMemberRepository.cs b/Bottomly/Repositories/IMemberRepository.cs index a367107..7d53a4a 100644 --- a/Bottomly/Repositories/IMemberRepository.cs +++ b/Bottomly/Repositories/IMemberRepository.cs @@ -4,6 +4,7 @@ namespace Bottomly.Repositories; public interface IMemberRepository { + Task> GetAllAsync(); Task GetByUsernameAsync(string username); Task GetBySlackIdAsync(string slackId); Task> GetBySlackIdsAsync(IEnumerable slackIds); diff --git a/Bottomly/Repositories/MemberCachePopulator.cs b/Bottomly/Repositories/MemberCachePopulator.cs new file mode 100644 index 0000000..6a738ca --- /dev/null +++ b/Bottomly/Repositories/MemberCachePopulator.cs @@ -0,0 +1,16 @@ +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Logging; + +namespace Bottomly.Repositories; + +public class MemberCachePopulator(IMemberRepository repository, ILogger logger) : IHostedService +{ + public async Task StartAsync(CancellationToken cancellationToken) + { + logger.LogInformation("Warming member cache..."); + var members = await repository.GetAllAsync(); + logger.LogInformation("Member cache warmed with {Count} members.", members.Count); + } + + public Task StopAsync(CancellationToken cancellationToken) => Task.CompletedTask; +} diff --git a/Bottomly/Repositories/MemberRepository.cs b/Bottomly/Repositories/MemberRepository.cs index b6dca0f..b69a853 100644 --- a/Bottomly/Repositories/MemberRepository.cs +++ b/Bottomly/Repositories/MemberRepository.cs @@ -7,6 +7,9 @@ public class MemberRepository(IMongoDatabase database) : IMemberRepository { private readonly IMongoCollection _collection = database.GetCollection("member"); + public Task> GetAllAsync() => + _collection.Find(_ => true).ToListAsync(); + public async Task GetByUsernameAsync(string username) => await _collection.Find(m => m.Username == username).FirstOrDefaultAsync(); diff --git a/Bottomly/Slack/MessageEventHandlers/ConversationMessageHandling/ConversationMessageHandler.cs b/Bottomly/Slack/MessageEventHandlers/ConversationMessageHandling/ConversationMessageHandler.cs index 899a534..e462c86 100644 --- a/Bottomly/Slack/MessageEventHandlers/ConversationMessageHandling/ConversationMessageHandler.cs +++ b/Bottomly/Slack/MessageEventHandlers/ConversationMessageHandling/ConversationMessageHandler.cs @@ -1,4 +1,5 @@ using Bottomly.LlmBot; +using Bottomly.Models; using Bottomly.Repositories; using Microsoft.Extensions.Logging; using SlackNet; @@ -15,7 +16,15 @@ public class ConversationMessageHandler( ILogger logger ) : IMessageEventHandler { - public bool CanHandle(MessageEvent message) => message.Text.Contains("bottomly"); + private readonly Task _botMemberTask = memberRepository.GetByUsernameAsync("bottomly"); + + public bool CanHandle(MessageEvent message) + { + if (message.Text.Contains("bottomly")) return true; + return _botMemberTask.IsCompletedSuccessfully + && _botMemberTask.Result?.SlackId is { } botId + && message.Text.Contains($"<@{botId}>"); + } public async Task HandleAsync(MessageEvent message) { From 19c01afed3168caad8949d370f36bab7eaa89a44 Mon Sep 17 00:00:00 2001 From: "Owen.Morgan-Jones" Date: Thu, 19 Mar 2026 12:57:35 +0000 Subject: [PATCH 3/4] Fixes issue where slacknet is already replacing some userid tokens in message objects --- .azure/container-group.yml | 4 +--- Bottomly/Slack/MessageEventHandlers/LlmToggleHandler.cs | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/.azure/container-group.yml b/.azure/container-group.yml index f8f8090..5d7f760 100644 --- a/.azure/container-group.yml +++ b/.azure/container-group.yml @@ -35,8 +35,6 @@ properties: value: ${APPINSIGHTS_CONN_STR} - name: ConnectionStrings__bottomlymodel secureValue: "Endpoint=http://localhost:11434;Key=${OLLAMA_API_KEY}" - - name: EnableLlm - value: "true" - name: ollama properties: image: ollama/ollama:latest @@ -59,5 +57,5 @@ properties: protocol: TCP osType: Linux restartPolicy: Always -tags: {} +tags: { } type: Microsoft.ContainerInstance/containerGroups diff --git a/Bottomly/Slack/MessageEventHandlers/LlmToggleHandler.cs b/Bottomly/Slack/MessageEventHandlers/LlmToggleHandler.cs index 1d753a2..3b1ac63 100644 --- a/Bottomly/Slack/MessageEventHandlers/LlmToggleHandler.cs +++ b/Bottomly/Slack/MessageEventHandlers/LlmToggleHandler.cs @@ -26,7 +26,7 @@ public override bool CanHandle(MessageEvent message) => protected override async Task InvokeHandlerLogicAsync(MessageEvent message) { - var sender = await memberRepository.GetBySlackIdAsync(message.User); + var sender = await memberRepository.GetByUsernameAsync(message.User); if (sender?.Username != AuthorisedUser) { await SendMessageResponseAsync("You don't have permission to toggle the LLM.", message, true); From 51d5fa22f1bfd3eeae4ed8b92c8c9703b6e179e3 Mon Sep 17 00:00:00 2001 From: "Owen.Morgan-Jones" Date: Thu, 19 Mar 2026 13:07:28 +0000 Subject: [PATCH 4/4] Fixes unit tests to also use getbyusername --- .../LlmToggleHandlerTests.cs | 33 +++++++++---------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/Bottomly.Tests/Slack/MessageEventHandlers/LlmToggleHandlerTests.cs b/Bottomly.Tests/Slack/MessageEventHandlers/LlmToggleHandlerTests.cs index f71a4db..2bdfbd6 100644 --- a/Bottomly.Tests/Slack/MessageEventHandlers/LlmToggleHandlerTests.cs +++ b/Bottomly.Tests/Slack/MessageEventHandlers/LlmToggleHandlerTests.cs @@ -1,4 +1,3 @@ -using Bottomly.Configuration; using Bottomly.Models; using Bottomly.Repositories; using Bottomly.Slack; @@ -13,27 +12,25 @@ namespace Bottomly.Tests.Slack.MessageEventHandlers; public class LlmToggleHandlerTests { + private readonly LlmToggleHandler _handler; + private readonly Mock _mockBroker = new(); private readonly Mock _mockFlagRepo = new(); private readonly Mock _mockMemberRepo = new(); - private readonly Mock _mockBroker = new(); - private readonly LlmToggleHandler _handler; - public LlmToggleHandlerTests() - { + public LlmToggleHandlerTests() => _handler = new LlmToggleHandler( _mockFlagRepo.Object, _mockMemberRepo.Object, _mockBroker.Object, TestHelpers.CreateOptions(), NullLogger.Instance); - } - private static MessageEvent CreateMessage(string text, string user = "U_OWEN") => + private static MessageEvent CreateMessage(string text, string user = "owen") => new() { Text = text, User = user, Channel = "C1", Ts = "ts1" }; private void SetupMemberForSlackId(string slackId, string username) => _mockMemberRepo - .Setup(r => r.GetBySlackIdAsync(slackId)) + .Setup(r => r.GetByUsernameAsync(username)) .ReturnsAsync(new Member { SlackId = slackId, Username = username }); // ─── CanHandle ───────────────────────────────────────────────────────────── @@ -61,7 +58,7 @@ public async Task HandleAsync_NonOwenUser_RepliesWithPermissionDenied() { SetupMemberForSlackId("U_OTHER", "alice"); - await _handler.HandleAsync(CreateMessage("_llm on", "U_OTHER")); + await _handler.HandleAsync(CreateMessage("_llm on", "alice")); _mockBroker.Verify( b => b.SendMessageAsync(It.Is(s => s.Contains("permission")), "C1", It.IsAny()), @@ -89,7 +86,7 @@ public async Task HandleAsync_LlmOn_OwenUser_EnablesFlag() { SetupMemberForSlackId("U_OWEN", "owen"); - await _handler.HandleAsync(CreateMessage("_llm on", "U_OWEN")); + await _handler.HandleAsync(CreateMessage("_llm on")); _mockFlagRepo.Verify(r => r.SetAsync("EnableLlm", true), Times.Once()); } @@ -99,10 +96,11 @@ public async Task HandleAsync_LlmOn_OwenUser_ConfirmsEnabled() { SetupMemberForSlackId("U_OWEN", "owen"); - await _handler.HandleAsync(CreateMessage("_llm on", "U_OWEN")); + await _handler.HandleAsync(CreateMessage("_llm on")); _mockBroker.Verify( - b => b.SendMessageAsync(It.Is(s => s.Contains("enabled", StringComparison.OrdinalIgnoreCase)), "C1", It.IsAny()), + b => b.SendMessageAsync(It.Is(s => s.Contains("enabled", StringComparison.OrdinalIgnoreCase)), "C1", + It.IsAny()), Times.Once()); } @@ -113,7 +111,7 @@ public async Task HandleAsync_LlmOff_OwenUser_DisablesFlag() { SetupMemberForSlackId("U_OWEN", "owen"); - await _handler.HandleAsync(CreateMessage("_llm off", "U_OWEN")); + await _handler.HandleAsync(CreateMessage("_llm off")); _mockFlagRepo.Verify(r => r.SetAsync("EnableLlm", false), Times.Once()); } @@ -123,10 +121,11 @@ public async Task HandleAsync_LlmOff_OwenUser_ConfirmsDisabled() { SetupMemberForSlackId("U_OWEN", "owen"); - await _handler.HandleAsync(CreateMessage("_llm off", "U_OWEN")); + await _handler.HandleAsync(CreateMessage("_llm off")); _mockBroker.Verify( - b => b.SendMessageAsync(It.Is(s => s.Contains("disabled", StringComparison.OrdinalIgnoreCase)), "C1", It.IsAny()), + b => b.SendMessageAsync(It.Is(s => s.Contains("disabled", StringComparison.OrdinalIgnoreCase)), + "C1", It.IsAny()), Times.Once()); } @@ -137,11 +136,11 @@ public async Task HandleAsync_UnknownArg_OwenUser_RepliesWithUsageError() { SetupMemberForSlackId("U_OWEN", "owen"); - await _handler.HandleAsync(CreateMessage("_llm blah", "U_OWEN")); + await _handler.HandleAsync(CreateMessage("_llm blah")); _mockBroker.Verify( b => b.SendMessageAsync(It.Is(s => s.Contains("blah")), "C1", It.IsAny()), Times.Once()); _mockFlagRepo.Verify(r => r.SetAsync(It.IsAny(), It.IsAny()), Times.Never()); } -} +} \ No newline at end of file