From c313fe3db9a99ad759e0f8539cf8c6ef7915e88f Mon Sep 17 00:00:00 2001 From: Mah Noor Date: Thu, 6 Aug 2026 22:09:33 +0500 Subject: [PATCH] fix(exam-checker): stop casual mentions hijacking chat + always give an exit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A plain-text question — "Can you grade exam papers for me?" — silently started a real exam-grading session because shouldTriggerExamChecker matched the phrase "grade exam" ANYWHERE via .includes(). Once in that session every subsequent plain-text message was captured with no reply, and there was no timeout and no text way out (handleExamCancel was dead code, and a slash command bypassed the checker but left the session active, so the next message was recaptured — the same trap fixed for coaching in bd-2508). Fixes, all TDD (red-first, 14 new tests): - Anchored trigger: the message must BE or LEAD with a trigger phrase, not merely contain one. "grade exams" still starts it; a question mentioning it does not. - Explicit exit words (stop/cancel/exit + Urdu/Arabic) end the session and confirm; a slash command ends it silently then runs (mirrors bd-2508). - Accidental sessions (collecting_images, zero images, untouched >1h) auto- expire so they cannot linger and recapture chat. Baseline suite unchanged (44 pre-existing env failures before and after); +14 passing tests. Refs: bd-2484 Co-Authored-By: Claude Opus 4.8 (1M context) --- bot/shared/handlers/exam-checker.handler.js | 91 ++++++++++++-- bot/shared/handlers/text-message.handler.js | 29 +++++ .../exam-checker/exam-session.service.js | 40 +++++++ .../bd-2484-session-staleness.test.js | 58 +++++++++ .../bd-2484-trigger-and-exit.test.js | 112 ++++++++++++++++++ 5 files changed, 318 insertions(+), 12 deletions(-) create mode 100644 tests/exam-checker/bd-2484-session-staleness.test.js create mode 100644 tests/exam-checker/bd-2484-trigger-and-exit.test.js diff --git a/bot/shared/handlers/exam-checker.handler.js b/bot/shared/handlers/exam-checker.handler.js index 689cb2c8..e26305b5 100644 --- a/bot/shared/handlers/exam-checker.handler.js +++ b/bot/shared/handlers/exam-checker.handler.js @@ -28,20 +28,42 @@ const EXAM_CHECK_KEYWORDS = [ 'تصحيح امتحان', 'تصحيح الامتحان', 'تقييم امتحان' ]; +// Explicit exit words — the way OUT of an active exam-checker session. +// (English + Urdu + Arabic.) bd-2484: before this, the session had no text +// exit, so an accidental trigger trapped normal chat until a slash command. +const EXAM_EXIT_KEYWORDS = [ + // English + 'stop', 'cancel', 'exit', 'quit', 'end', + 'stop exam', 'cancel exam', 'stop grading', 'cancel grading', + // Urdu + 'روکیں', 'روکو', 'منسوخ', 'بند کرو', 'بند کریں', + // Arabic + 'إلغاء', 'الغاء', 'توقف', 'إيقاف', +]; + // Button prefixes for exam checker const EXAM_BUTTON_PREFIX = 'ech_'; /** - * Check if a text message should trigger exam checker + * Check if a text message should START exam checking. + * + * bd-2484: ANCHORED intent — the message must BE a trigger phrase or LEAD with + * one. The old behaviour matched a keyword anywhere in the text (`.includes`), + * so a casual mention buried in a question — "Can you grade exam papers for + * me?" — silently started a real grading session and hijacked normal chat. + * Starting a feature should require clear intent, not an incidental word. + * * @param {string} text - Message text * @returns {boolean} */ function shouldTriggerExamChecker(text) { if (!text) return false; - const normalizedText = text.toLowerCase().trim(); + // Lowercase, and drop trailing punctuation/space so "grade exams?" still counts. + const normalized = text.toLowerCase().trim().replace(/[?!.\s]+$/g, ''); for (const keyword of EXAM_CHECK_KEYWORDS) { - if (normalizedText.includes(keyword.toLowerCase())) { + const kw = keyword.toLowerCase(); + if (normalized === kw || normalized.startsWith(kw + ' ')) { return true; } } @@ -49,6 +71,19 @@ function shouldTriggerExamChecker(text) { return false; } +/** + * Check if a text message is an explicit request to LEAVE exam checking. + * Matches only the exit word itself (optionally punctuated) — never a sentence + * that merely contains "stop" ("how do I stop my students talking"). + * @param {string} text - Message text + * @returns {boolean} + */ +function isExamExitText(text) { + if (!text) return false; + const t = text.toLowerCase().trim().replace(/[?!.\s]+$/g, ''); + return EXAM_EXIT_KEYWORDS.includes(t); +} + /** * Check if a button click belongs to exam checker * @param {string} buttonId - Button ID @@ -351,30 +386,60 @@ async function handleExamFlow(flowId, flowResponse, from, user) { } /** - * Handle cancel command for exam checker + * End the user's active exam-checker session, if any. This is THE way out of + * the state that (before bd-2484) trapped normal chat. + * + * Two callers, two modes: + * - notify:true → the teacher typed an explicit exit word ("stop"/"cancel"). + * We end the session AND confirm it in chat. + * - notify:false → the teacher ran a slash command (/menu, /training…). We end + * the session SILENTLY and let the command run — mirrors the + * coaching escape fix (bd-2508): bypassing without ending the + * session lets the very next plain text get recaptured. + * * @param {string} from - Phone number * @param {Object} user - User object - * @returns {Promise} + * @param {{notify?: boolean}} [opts] + * @returns {Promise} true if a session was actually ended */ -async function handleExamCancel(from, user) { - if (!user) return { handled: false }; +async function endActiveExamSession(from, user, { notify = false } = {}) { + if (!user) return false; const state = await ExamCheckerOrchestrator.getSessionState(user.id); + if (!state.active) return false; - if (!state.active) { - return { handled: false }; + await ExamCheckerOrchestrator.cancelSession(state.sessionId); + + if (notify) { + await WhatsAppService.sendMessage( + from, + '✅ Exam checking cancelled. Say "check exams" whenever you\'re ready to grade papers.' + ); } - const response = await ExamCheckerOrchestrator.cancelSession(state.sessionId); - await WhatsAppService.sendMessage(from, response.text); + logToFile('🚪 Exam session ended via exit path', { + userId: user.id, sessionId: state.sessionId, notify, + }); + + return true; +} - return { handled: true }; +/** + * Handle cancel command for exam checker (thin wrapper over endActiveExamSession). + * @param {string} from - Phone number + * @param {Object} user - User object + * @returns {Promise} + */ +async function handleExamCancel(from, user) { + const handled = await endActiveExamSession(from, user, { notify: true }); + return { handled }; } module.exports = { // Detection functions shouldTriggerExamChecker, isExamCheckerButton, + isExamExitText, hasActiveExamSession, // Handler functions @@ -383,8 +448,10 @@ module.exports = { handleExamButton, handleExamFlow, handleExamCancel, + endActiveExamSession, // Constants EXAM_CHECK_KEYWORDS, + EXAM_EXIT_KEYWORDS, EXAM_BUTTON_PREFIX }; diff --git a/bot/shared/handlers/text-message.handler.js b/bot/shared/handlers/text-message.handler.js index 7bc185d8..1fdd2f60 100644 --- a/bot/shared/handlers/text-message.handler.js +++ b/bot/shared/handlers/text-message.handler.js @@ -427,6 +427,35 @@ async function handleTextMessage(message, from, messageBody, user = null) { return; // Stop further processing } + // ============================================================ + // EXAM CHECKER — ESCAPE PATH (bd-2484): a waiting state must never trap + // normal chat. Checked BEFORE the trigger/capture block below. + // • an explicit stop/cancel word ENDS the session and confirms it, then + // returns (the message is fully handled); + // • a slash command ENDS the session SILENTLY and falls through so the + // command still runs — mirrors the coaching fix (bd-2508): bypassing the + // checker without ending the session lets the next plain text get + // recaptured, so the teacher escapes and is immediately caught again. + // Both are cheap no-ops (a single Redis-first lookup) when no exam session + // is active, and only run for slash commands or literal exit words. + // ============================================================ + if (user) { + try { + const ExamCheckerHandler = require('./exam-checker.handler'); + const isSlash = trimmedMessage.startsWith('/'); + if (isSlash || ExamCheckerHandler.isExamExitText(trimmedMessage)) { + const ended = await ExamCheckerHandler.endActiveExamSession(from, user, { notify: !isSlash }); + if (ended && !isSlash) { + logToFile('🚪 Exam checker exited by stop/cancel word', { userId: user.id }); + typingController.stop(); + return; + } + } + } catch (error) { + logToFile('⚠️ Exam checker escape-path check failed (non-fatal)', { error: error.message }); + } + } + // ============================================================ // EXAM CHECKER DETECTION: Check for exam check trigger // diff --git a/bot/shared/services/exam-checker/exam-session.service.js b/bot/shared/services/exam-checker/exam-session.service.js index d1426695..c3619612 100644 --- a/bot/shared/services/exam-checker/exam-session.service.js +++ b/bot/shared/services/exam-checker/exam-session.service.js @@ -14,6 +14,11 @@ const { logToFile } = require('../../utils/logger'); const REDIS_PREFIX = 'exam_session:'; const REDIS_TTL = 60 * 60 * 24; // 24 hours +// bd-2484: an accidental session (started but never used) is auto-expired after +// this long so it can't linger and recapture normal chat. Only ever applied to +// a `collecting_images` session with zero images — never to work in progress. +const STALE_COLLECTING_MS = 60 * 60 * 1000; // 1 hour + class ExamSessionService { /** * Get or create an exam session for a user @@ -46,6 +51,39 @@ class ExamSessionService { return this._createSession(userId); } + /** + * True if this is an ACCIDENTAL session that should auto-expire: still + * collecting images, none uploaded, and untouched for over STALE_COLLECTING_MS. + * Never true once images exist or the session has advanced past collection. + * @param {object} session + * @returns {boolean} + */ + static _isStaleCollectingSession(session) { + if (!session || session.status !== 'collecting_images') return false; + if ((session.original_images || []).length > 0) return false; + const ts = Date.parse(session.updated_at || session.created_at || ''); + if (!Number.isFinite(ts)) return false; + return (Date.now() - ts) > STALE_COLLECTING_MS; + } + + /** + * If the session is a stale accidental one, cancel it (which clears Redis) + * and return true so callers treat it as "no active session". + * @param {object} session + * @returns {Promise} true if it was expired + */ + static async _expireIfStale(session) { + if (!this._isStaleCollectingSession(session)) return false; + try { + await this.updateStatus(session.id, 'cancelled', { error_message: 'auto_expired_stale_collecting' }); + logToFile('⏱️ Auto-expired stale exam session', { sessionId: session.id, userId: session.user_id }); + } catch (error) { + logToFile('⚠️ Failed to auto-expire stale exam session', { sessionId: session.id, error: error.message }); + await this._clearFromRedis(session.user_id); + } + return true; + } + /** * Get active session for user (without creating new one) * @param {string} userId - User UUID @@ -55,6 +93,7 @@ class ExamSessionService { // Check Redis first const cachedSession = await this._getFromRedis(userId); if (cachedSession) { + if (await this._expireIfStale(cachedSession)) return null; return cachedSession; } @@ -69,6 +108,7 @@ class ExamSessionService { .single(); if (data && !error) { + if (await this._expireIfStale(data)) return null; await this._saveToRedis(userId, data); return data; } diff --git a/tests/exam-checker/bd-2484-session-staleness.test.js b/tests/exam-checker/bd-2484-session-staleness.test.js new file mode 100644 index 00000000..3dda4a72 --- /dev/null +++ b/tests/exam-checker/bd-2484-session-staleness.test.js @@ -0,0 +1,58 @@ +/** + * bd-2484 — defense-in-depth: an ACCIDENTAL exam session (started but never + * used) must auto-expire, so it can't linger and recapture chat if the teacher + * never types an exit word or slash command. + * + * Guard is deliberately narrow: it only expires a session that is still in + * `collecting_images`, has ZERO images, and is untouched for over an hour. A + * session with images or one that has advanced past collection (a real grading + * in progress) is NEVER expired. + */ + +jest.mock('../../bot/shared/config/supabase', () => ({ from: jest.fn() })); +jest.mock('../../bot/shared/services/cache/railway-redis.service', () => ({ + redis: {}, + get: jest.fn(async () => null), + set: jest.fn(async () => {}), + delete: jest.fn(async () => {}), +})); +jest.mock('../../bot/shared/utils/logger', () => ({ logToFile: jest.fn() })); + +const ExamSessionService = require('../../bot/shared/services/exam-checker/exam-session.service'); + +const hoursAgo = (h) => new Date(Date.now() - h * 60 * 60 * 1000).toISOString(); + +describe('_isStaleCollectingSession', () => { + it('is stale: collecting_images, no images, untouched > 1h', () => { + expect(ExamSessionService._isStaleCollectingSession({ + status: 'collecting_images', original_images: [], updated_at: hoursAgo(2), + })).toBe(true); + }); + + it('is NOT stale when recent (within the hour)', () => { + expect(ExamSessionService._isStaleCollectingSession({ + status: 'collecting_images', original_images: [], updated_at: hoursAgo(0.2), + })).toBe(false); + }); + + it('is NOT stale when the teacher has already uploaded images', () => { + expect(ExamSessionService._isStaleCollectingSession({ + status: 'collecting_images', original_images: [{ url: 'x' }], updated_at: hoursAgo(5), + })).toBe(false); + }); + + it('is NOT stale once the session has advanced past collection', () => { + expect(ExamSessionService._isStaleCollectingSession({ + status: 'grading', original_images: [], updated_at: hoursAgo(5), + })).toBe(false); + }); + + it('falls back to created_at when updated_at is absent, and is safe on bad timestamps', () => { + expect(ExamSessionService._isStaleCollectingSession({ + status: 'collecting_images', original_images: [], created_at: hoursAgo(3), + })).toBe(true); + expect(ExamSessionService._isStaleCollectingSession({ + status: 'collecting_images', original_images: [], updated_at: 'not-a-date', + })).toBe(false); + }); +}); diff --git a/tests/exam-checker/bd-2484-trigger-and-exit.test.js b/tests/exam-checker/bd-2484-trigger-and-exit.test.js new file mode 100644 index 00000000..05d20426 --- /dev/null +++ b/tests/exam-checker/bd-2484-trigger-and-exit.test.js @@ -0,0 +1,112 @@ +/** + * bd-2484 — exam-checker must not hijack normal chat, and must always have an exit. + * + * Two faults, one report ("Can you grade exam papers for me?" bricked normal chat): + * 1. TRIGGER TOO LOOSE — `.includes('grade exam')` matched the phrase buried in a + * question, starting a real grading session. Fix: anchored intent (the message + * IS / STARTS WITH a trigger phrase), not substring-anywhere. + * 2. NO WAY OUT — no stop/cancel text exit; a slash command bypassed the checker + * but left the session active, so the next plain text was recaptured (the same + * trap fixed for coaching in bd-2508). Fix: an explicit exit word AND a slash + * command both END the active session. + * + * These lock the two seams in exam-checker.handler.js: shouldTriggerExamChecker + * (detection) and the exit helpers (isExamExitText / endActiveExamSession). + */ + +const mockCancelSession = jest.fn(async () => ({ text: 'cancelled' })); +const mockGetSessionState = jest.fn(async () => ({ active: false })); + +jest.mock('../../bot/shared/services/exam-checker', () => ({ + ExamCheckerOrchestrator: { + cancelSession: mockCancelSession, + getSessionState: mockGetSessionState, + process: jest.fn(), + }, + ExamSessionService: {}, +})); +jest.mock('../../bot/shared/services/whatsapp.service', () => ({ + startContinuousTypingIndicator: () => ({ stop: jest.fn() }), + sendMessage: jest.fn(async () => {}), + sendInteractiveMessage: jest.fn(async () => {}), + sendFlow: jest.fn(async () => {}), + downloadMedia: jest.fn(async () => Buffer.from('')), +})); +jest.mock('../../bot/shared/storage/r2', () => ({ uploadImageWithRetry: jest.fn(async () => 'url') })); +jest.mock('../../bot/shared/utils/logger', () => ({ logToFile: jest.fn() })); +jest.mock('../../bot/shared/utils/structured-logger', () => ({ + runWithCorrelation: (_id, fn) => fn(), + generateCorrelationId: () => 'cid', +})); + +const Handler = require('../../bot/shared/handlers/exam-checker.handler'); +const WhatsAppService = require('../../bot/shared/services/whatsapp.service'); + +beforeEach(() => { + jest.clearAllMocks(); + mockGetSessionState.mockResolvedValue({ active: false }); +}); + +describe('shouldTriggerExamChecker — anchored intent, not substring-anywhere', () => { + it('does NOT trigger on a casual mention buried in a question (the reported bug)', () => { + expect(Handler.shouldTriggerExamChecker('Can you grade exam papers for me?')).toBe(false); + }); + + it('does NOT trigger on other natural-language questions about grading', () => { + expect(Handler.shouldTriggerExamChecker('How do I grade exams for my class?')).toBe(false); + expect(Handler.shouldTriggerExamChecker('what is the best way to check exams')).toBe(false); + expect(Handler.shouldTriggerExamChecker('How do I keep grade 2 students focused?')).toBe(false); + }); + + it('DOES trigger when the message is (or starts with) a clear command phrase', () => { + expect(Handler.shouldTriggerExamChecker('grade exams')).toBe(true); + expect(Handler.shouldTriggerExamChecker('check exams')).toBe(true); + expect(Handler.shouldTriggerExamChecker('grade exam papers')).toBe(true); + expect(Handler.shouldTriggerExamChecker('check my papers')).toBe(true); + expect(Handler.shouldTriggerExamChecker('Grade Exams')).toBe(true); // case-insensitive + }); + + it('is empty/nullish safe', () => { + expect(Handler.shouldTriggerExamChecker('')).toBe(false); + expect(Handler.shouldTriggerExamChecker(undefined)).toBe(false); + }); +}); + +describe('isExamExitText — explicit stop/cancel exit', () => { + it('recognises stop/cancel/exit words (with trailing punctuation)', () => { + expect(Handler.isExamExitText('stop')).toBe(true); + expect(Handler.isExamExitText('cancel')).toBe(true); + expect(Handler.isExamExitText('exit')).toBe(true); + expect(Handler.isExamExitText('STOP!')).toBe(true); + }); + + it('does NOT treat a normal sentence that merely contains "stop" as an exit', () => { + expect(Handler.isExamExitText('how do I stop my students talking')).toBe(false); + expect(Handler.isExamExitText('grade exams')).toBe(false); + }); +}); + +describe('endActiveExamSession — the actual way out', () => { + it('cancels the session when one is active and reports it ended', async () => { + mockGetSessionState.mockResolvedValue({ active: true, sessionId: 'sess-1', state: 'collecting_images' }); + const ended = await Handler.endActiveExamSession('92300', { id: 'user-1' }, { notify: true }); + expect(ended).toBe(true); + expect(mockCancelSession).toHaveBeenCalledWith('sess-1'); + expect(WhatsAppService.sendMessage).toHaveBeenCalled(); // teacher told it's cancelled + }); + + it('is a no-op (returns false, no cancel) when there is no active session', async () => { + mockGetSessionState.mockResolvedValue({ active: false }); + const ended = await Handler.endActiveExamSession('92300', { id: 'user-1' }, { notify: true }); + expect(ended).toBe(false); + expect(mockCancelSession).not.toHaveBeenCalled(); + }); + + it('ends silently (no confirmation message) when notify is false — the slash-command path', async () => { + mockGetSessionState.mockResolvedValue({ active: true, sessionId: 'sess-2', state: 'collecting_images' }); + const ended = await Handler.endActiveExamSession('92300', { id: 'user-1' }, { notify: false }); + expect(ended).toBe(true); + expect(mockCancelSession).toHaveBeenCalledWith('sess-2'); + expect(WhatsAppService.sendMessage).not.toHaveBeenCalled(); + }); +});