From ecde7ee3326879629daf11908e2ee08618a59ed5 Mon Sep 17 00:00:00 2001 From: alex-mextner <111590326+alex-mextner@users.noreply.github.com> Date: Sat, 7 Nov 2026 07:51:58 +0100 Subject: [PATCH] fix(bot): keep multi-line payloads after an @mention; harden budget prompt flows (Refs #123) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mention route: (.+) stopped at the first newline, so the 2026-11-06 12:22 UTC budget list reached the AI as its first line only (chat_messages row 573, 68 chars) and the AI answered «Вы не указали сами лимиты». Extraction moved to extractMentionQuestion with ([\s\S]+); the index.ts wiring is pinned textually (createBot cannot be booted in a unit test). Budget-prompt hardening: suggestion buttons share the typed-path BUDGET_MAX_AMOUNT cap, a lost already_handled race in the text flow is consumed and logged instead of silently dropping the reply, and a successful typed apply strips the prompt's dead buttons via removeInlineKeyboard (best-effort, never thrown). --- src/bot/handlers/budget-callbacks.ts | 9 ++ src/bot/handlers/callback.handler.test.ts | 11 +++ src/bot/handlers/message.handler.test.ts | 27 ++++++ .../handlers/private-chat-redirect.test.ts | 1 + src/bot/index.ts | 11 +-- src/bot/mention-question.test.ts | 89 +++++++++++++++++++ src/bot/mention-question.ts | 16 ++++ src/bot/services/budget-prompt.test.ts | 40 ++++++++- src/bot/services/budget-prompt.ts | 18 +++- .../expense-flow.integration.test.ts | 1 + src/services/bank/telegram-sender.ts | 18 ++++ .../currency/budget-amount-parser.test.ts | 3 +- src/services/currency/budget-amount-parser.ts | 9 +- 13 files changed, 241 insertions(+), 12 deletions(-) create mode 100644 src/bot/mention-question.test.ts create mode 100644 src/bot/mention-question.ts diff --git a/src/bot/handlers/budget-callbacks.ts b/src/bot/handlers/budget-callbacks.ts index 63eef6c7..46e704e1 100644 --- a/src/bot/handlers/budget-callbacks.ts +++ b/src/bot/handlers/budget-callbacks.ts @@ -1,6 +1,7 @@ // Callback handlers for the smart budget prompt buttons: suggested amount, skip, show budget import type { Group, User } from '../../database/types'; +import { BUDGET_MAX_AMOUNT } from '../../services/currency/budget-amount-parser'; import { formatAmount } from '../../services/currency/converter'; import { sendBudgetView } from '../commands/budget'; import { @@ -42,6 +43,14 @@ async function handleSuggestion( return; } + // Same upper bound as typed amounts (parseBudgetInputAmount): both budget-prompt paths — + // typed replies and generated buttons — reject amounts at/above BUDGET_MAX_AMOUNT (#244). + // /budget set and the AI set_budget tool are separate paths with their own validation. + if (amount >= BUDGET_MAX_AMOUNT) { + await ctx.answerCallbackQuery({ text: '❌ Слишком большая сумма' }); + return; + } + const lookup = lookupPromptForCallback(promptId, group.id, user.id); if (!lookup.ok) { await ctx.answerCallbackQuery({ text: lookup.reason === 'stale' ? STALE_TEXT : FOREIGN_TEXT }); diff --git a/src/bot/handlers/callback.handler.test.ts b/src/bot/handlers/callback.handler.test.ts index 1a8343bf..8758b566 100644 --- a/src/bot/handlers/callback.handler.test.ts +++ b/src/bot/handlers/callback.handler.test.ts @@ -740,6 +740,17 @@ describe('handleCallbackQuery — routing table', () => { ); }); + test('a suggested amount at the 1e12 cap is rejected like a typed one (#244)', async () => { + const ctx = fakeCallbackCtx('budget:psuggest:5:1000000000000'); + await handleCallbackQuery(ctx as never, fakeBot() as never); + + expect(applyPromptMock).not.toHaveBeenCalled(); + expect(lookupPromptMock).not.toHaveBeenCalled(); + expect(ctx.answerCallbackQuery).toHaveBeenCalledWith( + expect.objectContaining({ text: expect.stringContaining('Слишком большая сумма') }), + ); + }); + test('an already handled prompt (double tap) keeps the message and reports stale', async () => { applyPromptMock.mockResolvedValueOnce('already_handled'); const ctx = fakeCallbackCtx('budget:psuggest:5:150'); diff --git a/src/bot/handlers/message.handler.test.ts b/src/bot/handlers/message.handler.test.ts index 58cbcf83..d46c0fc9 100644 --- a/src/bot/handlers/message.handler.test.ts +++ b/src/bot/handlers/message.handler.test.ts @@ -970,6 +970,33 @@ describe('production text route forwards budget intent intact', () => { expect(mockPendingExpenses.create).not.toHaveBeenCalled(); }); + // The exact 2026-11-06 12:22 UTC message from the issue-#123 follow-up: the AI answered + // «Вы не указали сами лимиты» because the @mention routing had truncated it to the first + // line. Without a mention the whole message must reach the AI verbatim — the history row + // it stores is what the follow-up turn («Как не указала, вот они») then reads. + test('exact November 2026 limit list reaches AI verbatim, including «Суммы в евро»', async () => { + const text = `Установи следующие лимиты по бюджету по категориям +Суммы в евро +Аренда 550 +Еда 900 +КУ 250 +Животные 200 +Машина 200 +Алекс 700 +Лена 700 +Прочее 200 +Подписки 450 +Накопления семья 700 +Путешествия 800`; + const ctx = fakeMessageCtx(text, { threadId: 77 }); + const bot = fakeBot(); + await routeTextMessage(ctx, bot, true); + expect(handleAskQuestionMock).toHaveBeenCalledTimes(1); + expect(handleAskQuestionMock).toHaveBeenCalledWith(ctx, text, bot); + expect(mockPendingExpenses.create).not.toHaveBeenCalled(); + expect(saveExpenseBatchMock).not.toHaveBeenCalled(); + }); + test('pure expenses are saved without invoking AI', async () => { await routeTextMessage( fakeMessageCtx('€450 — квартира\n\n€600 — еда', { threadId: 77 }), diff --git a/src/bot/handlers/private-chat-redirect.test.ts b/src/bot/handlers/private-chat-redirect.test.ts index c48b7ce0..e546a00c 100644 --- a/src/bot/handlers/private-chat-redirect.test.ts +++ b/src/bot/handlers/private-chat-redirect.test.ts @@ -17,6 +17,7 @@ mock.module('../../services/bank/telegram-sender', () => ({ withChatContext: mock(), initSender: mock(), editMessageText: mock(), + removeInlineKeyboard: mock(), deleteMessage: mock(), sendDirect: mock(), sendDocumentDirect: mock(), diff --git a/src/bot/index.ts b/src/bot/index.ts index 41ddcf7d..276edb22 100644 --- a/src/bot/index.ts +++ b/src/bot/index.ts @@ -40,6 +40,7 @@ import { requireAdmin, requireGoogle, requireGroup } from './guards'; import { handleCallbackQuery } from './handlers/callback.handler'; import { routeTextMessage } from './handlers/message.handler'; import { handlePhotoMessage } from './handlers/photo.handler'; +import { extractMentionQuestion } from './mention-question'; import { rateLimitOnResponseError, rateLimitPreRequest } from './rate-limit.hook'; import { sanitizeOutgoingMessages } from './sanitize-outgoing.hook'; import { registerTopicMiddleware } from './topic-middleware'; @@ -170,11 +171,11 @@ export function createBot(): Bot { // @mention is always the trigger for AI — works regardless of topics config if (botUsername) { - const mentionPattern = new RegExp(`@${botUsername}\\s+(.+)`, 'i'); - const match = text.match(mentionPattern); - - if (match?.[1]) { - await handleAskQuestion(ctx, match[1].trim(), bot, true); + // The question spans newlines: a mention followed by a multi-line budget list must + // reach the AI in full (2026-11-06 incident — the list was cut at the first line). + const question = extractMentionQuestion(text, botUsername); + if (question) { + await handleAskQuestion(ctx, question, bot, true); return; } } diff --git a/src/bot/mention-question.test.ts b/src/bot/mention-question.test.ts new file mode 100644 index 00000000..a667028c --- /dev/null +++ b/src/bot/mention-question.test.ts @@ -0,0 +1,89 @@ +// Regression tests for the mention-question extraction — pins the 2026-11-06 production +// incident (issue #123 follow-up): an @mention followed by a multi-line budget list was +// truncated to its first line and the AI answered «список … не дошёл». + +import { describe, expect, test } from 'bun:test'; +import { extractMentionQuestion } from './mention-question'; + +/** The exact message lari4kina_babich sent on 2026-11-06 12:22 UTC (chat_messages row 573 + * stored only its first line — 68 chars — everything after the newline was lost). */ +const NOVEMBER_BUDGET_LIST = `Установи следующие лимиты по бюджету по категориям +Суммы в евро +Аренда 550 +Еда 900 +КУ 250 +Животные 200 +Машина 200 +Алекс 700 +Лена 700 +Прочее 200 +Подписки 450 +Накопления семья 700 +Путешествия 800`; + +describe('extractMentionQuestion', () => { + test('keeps every line of the 2026-11-06 budget list after the mention', () => { + const question = extractMentionQuestion( + `@ExpenseSyncBot ${NOVEMBER_BUDGET_LIST}`, + 'ExpenseSyncBot', + ); + expect(question).toBe(NOVEMBER_BUDGET_LIST); + }); + + test('keeps the «Суммы в евро» currency note and the category-first line order verbatim', () => { + const question = extractMentionQuestion( + `@ExpenseSyncBot ${NOVEMBER_BUDGET_LIST}`, + 'ExpenseSyncBot', + ); + expect(question).toContain('Суммы в евро'); + expect(question).toContain('Аренда 550'); + expect(question).not.toContain('550 Аренда'); + expect(question?.split('\n')).toHaveLength(13); + }); + + test('a mention in the middle of the message keeps the multi-line remainder', () => { + const question = extractMentionQuestion( + `Ребята, давайте зафиксируем\n@ExpenseSyncBot ${NOVEMBER_BUDGET_LIST}`, + 'ExpenseSyncBot', + ); + expect(question).toBe(NOVEMBER_BUDGET_LIST); + }); + + test('the bot name matches case-insensitively', () => { + expect(extractMentionQuestion(`@expensesyncbot Аренда 550\nЕда 900`, 'ExpenseSyncBot')).toBe( + 'Аренда 550\nЕда 900', + ); + }); + + test('a bare mention with no payload is not a question', () => { + // The follow-up turn of the incident: «@ExpenseSyncBot» alone (row 580) must fall + // through to the normal text routing, not produce an empty AI question. + expect(extractMentionQuestion('@ExpenseSyncBot', 'ExpenseSyncBot')).toBeNull(); + expect(extractMentionQuestion('@ExpenseSyncBot \n\n', 'ExpenseSyncBot')).toBeNull(); + }); + + test('a message without the mention is left alone (plain-text routing handles it)', () => { + expect(extractMentionQuestion(NOVEMBER_BUDGET_LIST, 'ExpenseSyncBot')).toBeNull(); + expect(extractMentionQuestion('@ДругойБот Аренда 550', 'ExpenseSyncBot')).toBeNull(); + }); + + test('several prompts in one chat extract independently and statelessly', () => { + const first = extractMentionQuestion('@ExpenseSyncBot Аренда 550', 'ExpenseSyncBot'); + const second = extractMentionQuestion('@ExpenseSyncBot Еда 900\nКУ 250', 'ExpenseSyncBot'); + expect(first).toBe('Аренда 550'); + expect(second).toBe('Еда 900\nКУ 250'); + expect(extractMentionQuestion('@ExpenseSyncBot Аренда 550', 'ExpenseSyncBot')).toBe(first); + }); +}); + +// Source-contract pin: createBot() routes @mentions through extractMentionQuestion. The +// unit tests above cannot catch a regression that reintroduces an inline single-line +// `(.+)` regex in src/bot/index.ts (booting the real bot in a test is not viable), so the +// wiring is pinned textually — the incident fix must not be bypassable by an edit there. +describe('mention routing wiring', () => { + test('src/bot/index.ts extracts mention questions via extractMentionQuestion', async () => { + const source = await Bun.file(new URL('./index.ts', import.meta.url)).text(); + expect(source).toContain('extractMentionQuestion'); + expect(source).not.toContain(String.raw`\\s+(.+)`); + }); +}); diff --git a/src/bot/mention-question.ts b/src/bot/mention-question.ts new file mode 100644 index 00000000..c585f7d6 --- /dev/null +++ b/src/bot/mention-question.ts @@ -0,0 +1,16 @@ +// Extract the question text that follows a @botname mention. Kept as a leaf module so the +// mention routing (src/bot/index.ts) stays testable without booting the whole bot. + +/** + * Everything after the first "@ " in `text`, or null when the bot is not mentioned. + * + * The captured text spans newlines on purpose: a mention followed by a multi-line payload + * (a budget list, a pasted receipt, …) must reach the AI in full. The 2026-11-06 incident + * (issue #123 follow-up): `(.+)` without the dotall behaviour stopped at the first newline, + * the whole list was dropped and the AI answered «список … не дошёл». + */ +export function extractMentionQuestion(text: string, botUsername: string): string | null { + const match = text.match(new RegExp(`@${botUsername}\\s+([\\s\\S]+)`, 'i')); + const question = match?.[1]?.trim(); + return question ? question : null; +} diff --git a/src/bot/services/budget-prompt.test.ts b/src/bot/services/budget-prompt.test.ts index e8ae8595..b5e7f1cb 100644 --- a/src/bot/services/budget-prompt.test.ts +++ b/src/bot/services/budget-prompt.test.ts @@ -1,6 +1,6 @@ // Tests for the persistent budget prompt service: sending, plain input, reply disambiguation, claiming -import { mock } from 'bun:test'; +import { mock, spyOn } from 'bun:test'; // ── Real in-memory persistence for prompts, stubs for everything else ── import { BudgetPromptRepository } from '../../database/repositories/budget-prompt.repository'; import { GroupRepository } from '../../database/repositories/group.repository'; @@ -39,7 +39,11 @@ const sendMessageMock = mock( (_text: string, _options?: { reply_markup?: unknown }): Promise<{ message_id: number } | null> => Promise.resolve({ message_id: nextMessageId++ }), ); -mock.module('../../services/bank/telegram-sender', () => ({ sendMessage: sendMessageMock })); +const removeInlineKeyboardMock = mock((_messageId: number): Promise => Promise.resolve()); +mock.module('../../services/bank/telegram-sender', () => ({ + sendMessage: sendMessageMock, + removeInlineKeyboard: removeInlineKeyboardMock, +})); const setBudgetMock = mock( (_params: { @@ -106,6 +110,8 @@ beforeEach(() => { user = userRepo.create({ telegram_id: 1, group_id: group.id }); otherUser = userRepo.create({ telegram_id: 2, group_id: group.id }); nextMessageId = 1000; + removeInlineKeyboardMock.mockClear(); + logMock.debug.mockClear(); sendMessageMock.mockClear(); setBudgetMock.mockClear(); setBudgetMock.mockImplementation(() => Promise.resolve({ sheetsSynced: true })); @@ -179,6 +185,36 @@ describe('handleBudgetPromptText', () => { expect(keyboardData(options)).toContain('budget:view'); }); + test('a typed apply strips the prompt message buttons so they cannot be pressed later (#244)', async () => { + const id = await openPrompt('Food'); + const boundMessageId = prompts.findById(id)?.telegram_message_id; + expect(boundMessageId).not.toBeNull(); + + expect(await handleBudgetPromptText(input('150'))).toBe(true); + + expect(removeInlineKeyboardMock).toHaveBeenCalledTimes(1); + expect(removeInlineKeyboardMock).toHaveBeenCalledWith(boundMessageId); + }); + + test('the already_handled race consumes the reply, writes no budget and logs the prompt id (#244)', async () => { + const id = await openPrompt('Food'); + // The prompt is active at lookup, but a racing button claims it before our apply: + // finish() reports another writer won. + sendMessageMock.mockClear(); + const finishSpy = spyOn(prompts, 'finish').mockReturnValueOnce(false); + + expect(await handleBudgetPromptText(input('150'))).toBe(true); + + expect(setBudgetMock).not.toHaveBeenCalled(); + expect(sendMessageMock).not.toHaveBeenCalled(); + expect(removeInlineKeyboardMock).not.toHaveBeenCalled(); + expect(logMock.debug).toHaveBeenCalledWith( + expect.objectContaining({ promptId: id }), + expect.stringContaining('already_handled'), + ); + finishSpy.mockRestore(); + }); + test('an explicit currency overrides the prompt currency', async () => { await openPrompt('Food'); await handleBudgetPromptText(input('1 500 RSD')); diff --git a/src/bot/services/budget-prompt.ts b/src/bot/services/budget-prompt.ts index 81f02852..6dfb2b43 100644 --- a/src/bot/services/budget-prompt.ts +++ b/src/bot/services/budget-prompt.ts @@ -4,7 +4,7 @@ import { format } from 'date-fns'; import type { CurrencyCode } from '../../config/constants'; import { database } from '../../database'; import type { BudgetPrompt, Group, User } from '../../database/types'; -import { sendMessage } from '../../services/bank/telegram-sender'; +import { removeInlineKeyboard, sendMessage } from '../../services/bank/telegram-sender'; import { getBudgetManager } from '../../services/budget-manager'; import { type BudgetSuggestions, @@ -233,8 +233,24 @@ export async function handleBudgetPromptText(input: PromptTextInput): Promise ({ sendMessage: sendMessageMock, withChatContext: async (_c: number, _t: number | null, fn: () => Promise) => fn(), editMessageText: mock(() => Promise.resolve()), + removeInlineKeyboard: mock(() => Promise.resolve()), sendDirect: mock(() => Promise.resolve(null)), sendChatAction: mock(() => Promise.resolve()), deleteMessage: mock(() => Promise.resolve()), diff --git a/src/services/bank/telegram-sender.ts b/src/services/bank/telegram-sender.ts index 0884dd97..f2ba290f 100644 --- a/src/services/bank/telegram-sender.ts +++ b/src/services/bank/telegram-sender.ts @@ -90,6 +90,24 @@ export async function editMessageText( } } +/** + * Strip a message's inline keyboard, leaving its text as is. Reads chatId from context. + * A failure is logged, never thrown — dead buttons are cosmetic, the flow already succeeded. + */ +export async function removeInlineKeyboard(messageId: number): Promise { + try { + const { chatId } = getContext(); + await getBot().api.editMessageReplyMarkup({ + chat_id: chatId, + message_id: messageId, + reply_markup: { inline_keyboard: [] }, + }); + } catch (error) { + // Includes a missing chat context: the whole call is best-effort, never thrown. + logger.warn({ err: error, messageId }, 'removeInlineKeyboard failed'); + } +} + /** Delete a message. Reads chatId from context. */ export async function deleteMessage(messageId: number): Promise { const { chatId } = getContext(); diff --git a/src/services/currency/budget-amount-parser.test.ts b/src/services/currency/budget-amount-parser.test.ts index 7ae4f03f..eac019d1 100644 --- a/src/services/currency/budget-amount-parser.test.ts +++ b/src/services/currency/budget-amount-parser.test.ts @@ -9,7 +9,7 @@ describe('parseBudgetInputAmount', () => { ['150', 150, null], ['150 eur', 150, 'EUR'], ['150 EUR', 150, 'EUR'], - ['1 500 RSD', 1500, 'RSD'], + ['1\u00A0500 RSD', 1500, 'RSD'], // NBSP thousands separator — Telegram soft-wrap inserts it ['1500.50', 1500.5, null], ['1500,5', 1500.5, null], ['$150', 150, 'USD'], @@ -33,7 +33,6 @@ describe('parseBudgetInputAmount', () => { '$150 eur', '12abc', '1.2.3', - '99999999999999', '150\n200', ])('rejects %p', (input) => { expect(parseBudgetInputAmount(input)).toBeNull(); diff --git a/src/services/currency/budget-amount-parser.ts b/src/services/currency/budget-amount-parser.ts index 9c31f75f..cbbfea08 100644 --- a/src/services/currency/budget-amount-parser.ts +++ b/src/services/currency/budget-amount-parser.ts @@ -8,8 +8,13 @@ export interface ParsedBudgetAmount { currency: CurrencyCode | null; } -const MAX_AMOUNT = 1_000_000_000_000; -const NUMBER = String.raw`(\d{1,3}(?:[  ]\d{3})+(?:[.,]\d{1,2})?|\d+(?:[.,]\d{1,2})?)`; +/** Upper bound for budget-prompt amounts, typed (parseBudgetInputAmount) and suggestion + * buttons alike — one constant so both paths reject at the same line (#244). */ +export const BUDGET_MAX_AMOUNT = 1_000_000_000_000; + +const MAX_AMOUNT = BUDGET_MAX_AMOUNT; +// Space or NBSP separates thousands groups ("1 500", "1\u00A0500"); \d cannot stand in for it. +const NUMBER = String.raw`(\d{1,3}(?:[ ${'\u00A0'}]\d{3})+(?:[.,]\d{1,2})?|\d+(?:[.,]\d{1,2})?)`; const PREFIX_FORM = new RegExp(String.raw`^([$€£₽¥])\s*${NUMBER}$`); const SUFFIX_FORM = new RegExp(String.raw`^${NUMBER}\s*([$€£₽¥]|[\p{L}]+)?$`, 'u');