Fix: NOT SPAM button in potential spam was banning users (#98)

For potential spam, both KILL and NOT SPAM buttons shared the same callback with Spam type. Now each button has its own callback with correct type. Added tests and helper for cleanup of related callbacks.

Author: Szer

src/VahterBanBot.Tests/ContainerTestBase.fs

@@ -247,6 +247,14 @@ type VahterTestContainers(mlEnabled: bool) =
         return count > 0
     }
 
+    member _.UserBanned(userId: int64) = task {
+        use conn = new NpgsqlConnection(publicConnectionString)
+        //language=postgresql
+        let sql = "SELECT COUNT(*) FROM banned WHERE banned_user_id = @userId"
+        let! count = conn.QuerySingleAsync<int>(sql, {| userId = userId |})
+        return count > 0
+    }
+    
     member _.MessageIsAutoDeleted(msg: Message) = task {
         use conn = new NpgsqlConnection(publicConnectionString)
         //language=postgresql

src/VahterBanBot.Tests/MLBanTests.fs

@@ -82,7 +82,7 @@ type MLBanTests(fixture: MlEnabledVahterTestContainers, _unused: MlAwaitFixture)
 
     [<Fact>]
     let ``If message got auto-deleted we can mark it as false-positive with a button click`` () = task {
-        // record a message, where 2 is in a training set as spam word
+        // record a message, where 7 is in a training set as spam word (detected spam)
         // ChatsToMonitor[0] doesn't have stopwords
         let msgUpdate = Tg.quickMsg(chat = fixture.ChatsToMonitor[0], text = "7777777")
         let! _ = fixture.SendMessage msgUpdate
@@ -93,15 +93,50 @@ type MLBanTests(fixture: MlEnabledVahterTestContainers, _unused: MlAwaitFixture)
         // assert it is not false-positive
         let! isFalsePositive = fixture.IsMessageFalsePositive msgUpdate.Message
         Assert.False isFalsePositive
+        // assert user is NOT banned (auto-delete is not the same as ban)
+        let! userBanned = fixture.UserBanned msgUpdate.Message.From.Id
+        Assert.False userBanned
 
-        // send a callback to mark it as false-positive
+        // send a callback to mark it as false-positive (NOT SPAM button)
         let! callbackId = fixture.GetCallbackId msgUpdate.Message "NotASpam"
         let msgCallback = Tg.callback(string callbackId, from = fixture.Vahters[0])
         let! _ = fixture.SendMessage msgCallback
 
         // assert it is false-positive
         let! isFalsePositive = fixture.IsMessageFalsePositive msgUpdate.Message
         Assert.True isFalsePositive
+        // CRITICAL: assert user is still NOT banned after NOT SPAM button
+        let! userBanned = fixture.UserBanned msgUpdate.Message.From.Id
+        Assert.False userBanned
+    }
+    
+    [<Fact>]
+    let ``Potential spam NOT SPAM button does not ban user`` () = task {
+        // For potential spam, we need a text that gives score >= 0 but < 1.0 (ML_SPAM_THRESHOLD)
+        // Using a shorter spam-like text that might give lower score
+        // This tests the fix for the bug where NOT SPAM button was calling vahterMarkedAsSpam
+        let user = Tg.user()
+        let msgUpdate = Tg.quickMsg(chat = fixture.ChatsToMonitor[0], text = "77", from = user)
+        let! _ = fixture.SendMessage msgUpdate
+
+        // For potential spam, message is NOT auto-deleted (deleteMessage = false)
+        let! msgAutoDeleted = fixture.MessageIsAutoDeleted msgUpdate.Message
+        // If this is potential spam, it won't be auto-deleted
+        // If it IS auto-deleted, it means score >= threshold, but the test still validates the bug fix
+        
+        // User should NOT be banned initially
+        let! userBannedBefore = fixture.UserBanned user.Id
+        Assert.False userBannedBefore
+        
+        // Try to get NotASpam callback (this exists for both detected and potential spam)
+        let! callbackId = fixture.GetCallbackId msgUpdate.Message "NotASpam"
+        let msgCallback = Tg.callback(string callbackId, from = fixture.Vahters[0])
+        let! _ = fixture.SendMessage msgCallback
+        
+        // CRITICAL: User should still NOT be banned after NOT SPAM button
+        // This is the main assertion for the bug fix
+        let! userBannedAfter = fixture.UserBanned user.Id
+        Assert.False userBannedAfter
     }
 
     [<Fact>]

src/VahterBanBot/Bot.fs

@@ -484,38 +484,42 @@ let killSpammerAutomated
     let msgType = if deleteMessage then "Deleted" else "Detected"
     let logMsg = $"{msgType} spam (score: {score}) in {prependUsername message.Chat.Username} ({message.Chat.Id}) from {prependUsername message.From.Username} ({message.From.Id}) with text:\n{message.TextOrCaption}"
 
-    // Determine which topic and what buttons
-    let topicId, replyMarkup =
+    // Determine which topic
+    let topicId =
+        if deleteMessage then botConfig.ActionDetectedTopicId
+        else botConfig.ActionPotentialTopicId
+
+    // Phase 1: Create callback(s) without message_id
+    // For detected spam: one NotASpam callback
+    // For potential spam: two callbacks - Spam (KILL) and NotASpam (NOT SPAM)
+    let! callbackIds, markup = task {
         if deleteMessage then
-            // Detected spam → Detected topic with NOT A SPAM button
-            botConfig.ActionDetectedTopicId,
-            fun (callbackId: Guid) -> InlineKeyboardMarkup [
-                InlineKeyboardButton.WithCallbackData("✅ NOT a spam", string callbackId)
+            // Detected spam → one NotASpam callback
+            let! callback = DB.newCallbackPending (CallbackMessage.NotASpam { message = message }) message.From.Id topicId
+            return [callback.id], InlineKeyboardMarkup [
+                InlineKeyboardButton.WithCallbackData("✅ NOT a spam", string callback.id)
             ]
         else
-            // Potential spam → Potential topic with KILL and NOT SPAM buttons
-            botConfig.ActionPotentialTopicId,
-            fun (callbackId: Guid) -> InlineKeyboardMarkup [|
-                InlineKeyboardButton.WithCallbackData("🚫 KILL", string callbackId);
-                InlineKeyboardButton.WithCallbackData("✅ NOT SPAM", string callbackId)
+            // Potential spam → two callbacks
+            let! killCallback = DB.newCallbackPending (CallbackMessage.Spam { message = message }) message.From.Id topicId
+            let! notSpamCallback = DB.newCallbackPending (CallbackMessage.NotASpam { message = message }) message.From.Id topicId
+            return [killCallback.id; notSpamCallback.id], InlineKeyboardMarkup [|
+                InlineKeyboardButton.WithCallbackData("🚫 KILL", string killCallback.id);
+                InlineKeyboardButton.WithCallbackData("✅ NOT SPAM", string notSpamCallback.id)
             |]
-
-    // Phase 1: Create callback without message_id
-    let callbackData = 
-        if deleteMessage then CallbackMessage.NotASpam { message = message }
-        else CallbackMessage.Spam { message = message }
-    let! callback = DB.newCallbackPending callbackData message.From.Id topicId
+    }
 
     // Phase 2: Post to action channel topic
     let! sent = botClient.SendTextMessageAsync(
         chatId = ChatId(botConfig.ActionChannelId),
         text = logMsg,
         messageThreadId = topicId,
-        replyMarkup = replyMarkup callback.id
+        replyMarkup = markup
     )
 
-    // Phase 3: Update callback with message_id
-    do! DB.updateCallbackMessageId callback.id sent.MessageId
+    // Phase 3: Update all callbacks with message_id
+    for callbackId in callbackIds do
+        do! DB.updateCallbackMessageId callbackId sent.MessageId
 
     // Send to All Logs topic (readonly, no buttons)
     do! botClient.SendTextMessageAsync(
@@ -932,8 +936,12 @@ let onCallbackAux
         do! botClient.AnswerCallbackQueryAsync(callbackQuery.Id, "Already handled by another vahter")
 
     // Always delete message from action channel (empty inbox)
+    // and cleanup related callbacks (for potential spam with two buttons)
     match dbCallback.action_message_id with
     | Some msgId ->
+        // Delete other callbacks with same message_id (the other button)
+        do! DB.deleteCallbacksByMessageId msgId
+        // Delete message from channel
         do! botClient.DeleteMessageAsync(
                 ChatId(botConfig.ActionChannelId),
                 msgId

src/VahterBanBot/DB.fs

@@ -332,6 +332,18 @@ let deleteCallback (id: Guid): Task =
         return ()
     }
 
+/// Deletes all callbacks with the same action_message_id (for potential spam with two buttons)
+let deleteCallbacksByMessageId (actionMessageId: int): Task =
+    task {
+        use conn = new NpgsqlConnection(connString)
+
+        //language=postgresql
+        let sql = "DELETE FROM callback WHERE action_message_id = @msgId"
+
+        let! _ = conn.ExecuteAsync(sql, {| msgId = actionMessageId |})
+        return ()
+    }
+
 /// Gets all callbacks for a user (for cleanup when user is banned via /ban)
 let getCallbacksByUserId (userId: int64): Task<DbCallback array> =
     task {