From d107b79c6319944628f9fd56d797342ffff68005 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Tue, 2 Dec 2025 05:54:31 +0000 Subject: [PATCH] Fix test corrupting production sessions.json The test 'falls back to most recent session when no to is provided' was using resolveStorePath() which returns the real ~/.warelay/sessions.json. This overwrote production session data with test values, causing session fragmentation issues. Changed to use a temp directory like other tests. --- src/auto-reply/command-reply.test.ts | 75 ++++++++++++ src/auto-reply/command-reply.ts | 13 +- src/web/auto-reply.test.ts | 171 ++++++++++++++++++++++++--- src/web/monitor-inbox.test.ts | 160 +++++++++++++++++++++++-- src/web/test-helpers.ts | 31 +++-- 5 files changed, 412 insertions(+), 38 deletions(-) diff --git a/src/auto-reply/command-reply.test.ts b/src/auto-reply/command-reply.test.ts index fb47c0414..db1e64767 100644 --- a/src/auto-reply/command-reply.test.ts +++ b/src/auto-reply/command-reply.test.ts @@ -292,4 +292,79 @@ describe("runCommandReply", () => { expect(meta.queuedMs).toBe(25); expect(meta.queuedAhead).toBe(2); }); + + it("handles empty result string without dumping raw JSON", async () => { + // Bug fix: Claude CLI returning {"result": ""} should not send raw JSON to WhatsApp + // The fix changed from truthy check to explicit typeof check + const runner = makeRunner({ + stdout: '{"result":"","duration_ms":50,"total_cost_usd":0.001}', + }); + const { payload } = await runCommandReply({ + reply: { + mode: "command", + command: ["claude", "{{Body}}"], + claudeOutputFormat: "json", + }, + templatingCtx: noopTemplateCtx, + sendSystemOnce: false, + isNewSession: true, + isFirstTurnInSession: true, + systemSent: false, + timeoutMs: 1000, + timeoutSeconds: 1, + commandRunner: runner, + enqueue: enqueueImmediate, + }); + // Should NOT contain raw JSON - empty result should produce fallback message + expect(payload?.text).not.toContain('{"result"'); + expect(payload?.text).toContain("command produced no output"); + }); + + it("handles empty text string in Claude JSON", async () => { + const runner = makeRunner({ + stdout: '{"text":"","duration_ms":50}', + }); + const { payload } = await runCommandReply({ + reply: { + mode: "command", + command: ["claude", "{{Body}}"], + claudeOutputFormat: "json", + }, + templatingCtx: noopTemplateCtx, + sendSystemOnce: false, + isNewSession: true, + isFirstTurnInSession: true, + systemSent: false, + timeoutMs: 1000, + timeoutSeconds: 1, + commandRunner: runner, + enqueue: enqueueImmediate, + }); + // Empty text should produce fallback message, not raw JSON + expect(payload?.text).not.toContain('{"text"'); + expect(payload?.text).toContain("command produced no output"); + }); + + it("returns actual text when result is non-empty", async () => { + const runner = makeRunner({ + stdout: '{"result":"hello world","duration_ms":50}', + }); + const { payload } = await runCommandReply({ + reply: { + mode: "command", + command: ["claude", "{{Body}}"], + claudeOutputFormat: "json", + }, + templatingCtx: noopTemplateCtx, + sendSystemOnce: false, + isNewSession: true, + isFirstTurnInSession: true, + systemSent: false, + timeoutMs: 1000, + timeoutSeconds: 1, + commandRunner: runner, + enqueue: enqueueImmediate, + }); + expect(payload?.text).toBe("hello world"); + }); }); diff --git a/src/auto-reply/command-reply.ts b/src/auto-reply/command-reply.ts index 4d8b7141f..71e564948 100644 --- a/src/auto-reply/command-reply.ts +++ b/src/auto-reply/command-reply.ts @@ -259,8 +259,11 @@ export async function runCommandReply( console.error( `Command auto-reply exited with code ${code ?? "unknown"} (signal: ${signal ?? "none"})`, ); + // Include any partial output or stderr in error message + const partialOut = trimmed ? `\n\nOutput: ${trimmed.slice(0, 500)}${trimmed.length > 500 ? "..." : ""}` : ""; + const errorText = `⚠️ Command exited with code ${code ?? "unknown"}${signal ? ` (${signal})` : ""}${partialOut}`; return { - payload: undefined, + payload: { text: errorText }, meta: { durationMs: Date.now() - started, queuedMs, @@ -278,8 +281,9 @@ export async function runCommandReply( console.error( `Command auto-reply process killed before completion (exit code ${code ?? "unknown"})`, ); + const errorText = `⚠️ Command was killed before completion (exit code ${code ?? "unknown"})`; return { - payload: undefined, + payload: { text: errorText }, meta: { durationMs: Date.now() - started, queuedMs, @@ -379,8 +383,11 @@ export async function runCommandReply( }; } logError(`Command auto-reply failed after ${elapsed}ms: ${String(err)}`); + // Send error message to user so they know the command failed + const errMsg = err instanceof Error ? err.message : String(err); + const errorText = `⚠️ Command failed: ${errMsg}`; return { - payload: undefined, + payload: { text: errorText }, meta: { durationMs: elapsed, queuedMs, diff --git a/src/web/auto-reply.test.ts b/src/web/auto-reply.test.ts index 1918b5619..ff59e05d9 100644 --- a/src/web/auto-reply.test.ts +++ b/src/web/auto-reply.test.ts @@ -1,3 +1,10 @@ +// Import test-helpers FIRST to set up mocks before other imports +import { + resetBaileysMocks, + resetLoadConfigMock, + setLoadConfigMock, +} from "./test-helpers.js"; + import crypto from "node:crypto"; import fs from "node:fs/promises"; import os from "node:os"; @@ -18,11 +25,6 @@ import { stripHeartbeatToken, } from "./auto-reply.js"; import type { sendMessageWeb } from "./outbound.js"; -import { - resetBaileysMocks, - resetLoadConfigMock, - setLoadConfigMock, -} from "./test-helpers.js"; describe("heartbeat helpers", () => { it("strips heartbeat token and skips when only token", () => { @@ -186,6 +188,11 @@ describe("runWebHeartbeatOnce", () => { }); it("falls back to most recent session when no to is provided", async () => { + // Use temp directory to avoid corrupting production sessions.json + const tmpDir = await fs.mkdtemp( + path.join(os.tmpdir(), "warelay-fallback-session-"), + ); + const storePath = path.join(tmpDir, "sessions.json"); const sender: typeof sendMessageWeb = vi .fn() .mockResolvedValue({ messageId: "m1", toJid: "jid" }); @@ -196,15 +203,11 @@ describe("runWebHeartbeatOnce", () => { "+1222": { sessionId: "s1", updatedAt: now - 1000 }, "+1333": { sessionId: "s2", updatedAt: now }, }; - const storePath = resolveStorePath(); - await fs.mkdir(resolveStorePath().replace("sessions.json", ""), { - recursive: true, - }); await fs.writeFile(storePath, JSON.stringify(store)); setLoadConfigMock({ inbound: { allowFrom: ["+1999"], - reply: { mode: "command", session: {} }, + reply: { mode: "command", session: { store: storePath } }, }, }); await runWebHeartbeatOnce({ @@ -947,6 +950,16 @@ describe("web auto-reply", () => { }); it("prefixes body with same-phone marker when from === to", async () => { + // Enable messagePrefix for same-phone mode testing + setLoadConfigMock(() => ({ + inbound: { + allowFrom: ["*"], + messagePrefix: "[same-phone]", + responsePrefix: undefined, + timestampPrefix: false, + }, + })); + let capturedOnMessage: | ((msg: import("./inbound.js").WebInboundMessage) => Promise) | undefined; @@ -974,12 +987,11 @@ describe("web auto-reply", () => { sendMedia: vi.fn(), }); - // The resolver should receive a prefixed body (the exact marker depends on config) - // Key test: body should start with some marker and end with original message + // The resolver should receive a prefixed body with the configured marker const callArg = resolver.mock.calls[0]?.[0] as { Body?: string }; expect(callArg?.Body).toBeDefined(); - expect(callArg?.Body).toMatch(/^\[.*\] hello$/); - expect(callArg?.Body).not.toBe("hello"); // Should be prefixed + expect(callArg?.Body).toBe("[same-phone] hello"); + resetLoadConfigMock(); }); it("does not prefix body when from !== to", async () => { @@ -1014,4 +1026,135 @@ describe("web auto-reply", () => { const callArg = resolver.mock.calls[0]?.[0] as { Body?: string }; expect(callArg?.Body).toBe("hello"); }); + + it("applies responsePrefix to regular replies", async () => { + setLoadConfigMock(() => ({ + inbound: { + allowFrom: ["*"], + messagePrefix: undefined, + responsePrefix: "🦞", + timestampPrefix: false, + }, + })); + + let capturedOnMessage: + | ((msg: import("./inbound.js").WebInboundMessage) => Promise) + | undefined; + const reply = vi.fn(); + const listenerFactory = async (opts: { + onMessage: ( + msg: import("./inbound.js").WebInboundMessage, + ) => Promise; + }) => { + capturedOnMessage = opts.onMessage; + return { close: vi.fn() }; + }; + + const resolver = vi.fn().mockResolvedValue({ text: "hello there" }); + + await monitorWebProvider(false, listenerFactory, false, resolver); + expect(capturedOnMessage).toBeDefined(); + + await capturedOnMessage?.({ + body: "hi", + from: "+1555", + to: "+2666", + id: "msg1", + sendComposing: vi.fn(), + reply, + sendMedia: vi.fn(), + }); + + // Reply should have responsePrefix prepended + expect(reply).toHaveBeenCalledWith("🦞 hello there"); + resetLoadConfigMock(); + }); + + it("skips responsePrefix for HEARTBEAT_OK responses", async () => { + setLoadConfigMock(() => ({ + inbound: { + allowFrom: ["*"], + messagePrefix: undefined, + responsePrefix: "🦞", + timestampPrefix: false, + }, + })); + + let capturedOnMessage: + | ((msg: import("./inbound.js").WebInboundMessage) => Promise) + | undefined; + const reply = vi.fn(); + const listenerFactory = async (opts: { + onMessage: ( + msg: import("./inbound.js").WebInboundMessage, + ) => Promise; + }) => { + capturedOnMessage = opts.onMessage; + return { close: vi.fn() }; + }; + + // Resolver returns exact HEARTBEAT_OK + const resolver = vi.fn().mockResolvedValue({ text: HEARTBEAT_TOKEN }); + + await monitorWebProvider(false, listenerFactory, false, resolver); + expect(capturedOnMessage).toBeDefined(); + + await capturedOnMessage?.({ + body: "test", + from: "+1555", + to: "+2666", + id: "msg1", + sendComposing: vi.fn(), + reply, + sendMedia: vi.fn(), + }); + + // HEARTBEAT_OK should NOT have prefix - warelay needs exact match + expect(reply).toHaveBeenCalledWith(HEARTBEAT_TOKEN); + resetLoadConfigMock(); + }); + + it("does not double-prefix if responsePrefix already present", async () => { + setLoadConfigMock(() => ({ + inbound: { + allowFrom: ["*"], + messagePrefix: undefined, + responsePrefix: "🦞", + timestampPrefix: false, + }, + })); + + let capturedOnMessage: + | ((msg: import("./inbound.js").WebInboundMessage) => Promise) + | undefined; + const reply = vi.fn(); + const listenerFactory = async (opts: { + onMessage: ( + msg: import("./inbound.js").WebInboundMessage, + ) => Promise; + }) => { + capturedOnMessage = opts.onMessage; + return { close: vi.fn() }; + }; + + // Resolver returns text that already has prefix + const resolver = vi.fn().mockResolvedValue({ text: "🦞 already prefixed" }); + + await monitorWebProvider(false, listenerFactory, false, resolver); + expect(capturedOnMessage).toBeDefined(); + + await capturedOnMessage?.({ + body: "test", + from: "+1555", + to: "+2666", + id: "msg1", + sendComposing: vi.fn(), + reply, + sendMedia: vi.fn(), + }); + + // Should not double-prefix + expect(reply).toHaveBeenCalledWith("🦞 already prefixed"); + resetLoadConfigMock(); + }); }); diff --git a/src/web/monitor-inbox.test.ts b/src/web/monitor-inbox.test.ts index 10e312691..b696d2513 100644 --- a/src/web/monitor-inbox.test.ts +++ b/src/web/monitor-inbox.test.ts @@ -9,15 +9,17 @@ vi.mock("../media/store.js", () => ({ }), })); +const mockLoadConfig = vi.fn().mockReturnValue({ + inbound: { + allowFrom: ["*"], // Allow all in tests + messagePrefix: undefined, + responsePrefix: undefined, + timestampPrefix: false, + }, +}); + vi.mock("../config/config.js", () => ({ - loadConfig: vi.fn().mockReturnValue({ - inbound: { - allowFrom: ["*"], // Allow all in tests - messagePrefix: undefined, - responsePrefix: undefined, - timestampPrefix: false, - }, - }), + loadConfig: () => mockLoadConfig(), })); vi.mock("./session.js", () => { @@ -227,4 +229,146 @@ describe("web monitor inbox", () => { ]); await listener.close(); }); + + it("blocks messages from unauthorized senders not in allowFrom", async () => { + // Test for auto-recovery fix: early allowFrom filtering prevents Bad MAC errors + // from unauthorized senders corrupting sessions + mockLoadConfig.mockReturnValue({ + inbound: { + allowFrom: ["+111"], // Only allow +111 + messagePrefix: undefined, + responsePrefix: undefined, + timestampPrefix: false, + }, + }); + + const onMessage = vi.fn(); + const listener = await monitorWebInbox({ verbose: false, onMessage }); + const sock = await createWaSocket(); + + // Message from unauthorized sender +999 (not in allowFrom) + const upsert = { + type: "notify", + messages: [ + { + key: { id: "unauth1", fromMe: false, remoteJid: "999@s.whatsapp.net" }, + message: { conversation: "unauthorized message" }, + messageTimestamp: 1_700_000_000, + }, + ], + }; + + sock.ev.emit("messages.upsert", upsert); + await new Promise((resolve) => setImmediate(resolve)); + + // Should NOT call onMessage for unauthorized senders + expect(onMessage).not.toHaveBeenCalled(); + + // Reset mock for other tests + mockLoadConfig.mockReturnValue({ + inbound: { + allowFrom: ["*"], + messagePrefix: undefined, + responsePrefix: undefined, + timestampPrefix: false, + }, + }); + + await listener.close(); + }); + + it("allows messages from senders in allowFrom list", async () => { + mockLoadConfig.mockReturnValue({ + inbound: { + allowFrom: ["+111", "+999"], // Allow +999 + messagePrefix: undefined, + responsePrefix: undefined, + timestampPrefix: false, + }, + }); + + const onMessage = vi.fn(); + const listener = await monitorWebInbox({ verbose: false, onMessage }); + const sock = await createWaSocket(); + + const upsert = { + type: "notify", + messages: [ + { + key: { id: "auth1", fromMe: false, remoteJid: "999@s.whatsapp.net" }, + message: { conversation: "authorized message" }, + messageTimestamp: 1_700_000_000, + }, + ], + }; + + sock.ev.emit("messages.upsert", upsert); + await new Promise((resolve) => setImmediate(resolve)); + + // Should call onMessage for authorized senders + expect(onMessage).toHaveBeenCalledWith( + expect.objectContaining({ body: "authorized message", from: "+999" }), + ); + + // Reset mock for other tests + mockLoadConfig.mockReturnValue({ + inbound: { + allowFrom: ["*"], + messagePrefix: undefined, + responsePrefix: undefined, + timestampPrefix: false, + }, + }); + + await listener.close(); + }); + + it("allows same-phone messages even if not in allowFrom", async () => { + // Same-phone mode: when from === selfJid, should always be allowed + // This allows users to message themselves even with restrictive allowFrom + mockLoadConfig.mockReturnValue({ + inbound: { + allowFrom: ["+111"], // Only allow +111, but self is +123 + messagePrefix: undefined, + responsePrefix: undefined, + timestampPrefix: false, + }, + }); + + const onMessage = vi.fn(); + const listener = await monitorWebInbox({ verbose: false, onMessage }); + const sock = await createWaSocket(); + + // Message from self (sock.user.id is "123@s.whatsapp.net" in mock) + const upsert = { + type: "notify", + messages: [ + { + key: { id: "self1", fromMe: false, remoteJid: "123@s.whatsapp.net" }, + message: { conversation: "self message" }, + messageTimestamp: 1_700_000_000, + }, + ], + }; + + sock.ev.emit("messages.upsert", upsert); + await new Promise((resolve) => setImmediate(resolve)); + + // Should allow self-messages even if not in allowFrom + expect(onMessage).toHaveBeenCalledWith( + expect.objectContaining({ body: "self message", from: "+123" }), + ); + + // Reset mock for other tests + mockLoadConfig.mockReturnValue({ + inbound: { + allowFrom: ["*"], + messagePrefix: undefined, + responsePrefix: undefined, + timestampPrefix: false, + }, + }); + + await listener.close(); + }); }); diff --git a/src/web/test-helpers.ts b/src/web/test-helpers.ts index 8a0b02195..923fa19f3 100644 --- a/src/web/test-helpers.ts +++ b/src/web/test-helpers.ts @@ -3,32 +3,37 @@ import { vi } from "vitest"; import type { MockBaileysSocket } from "../../test/mocks/baileys.js"; import { createMockBaileys } from "../../test/mocks/baileys.js"; -let loadConfigMock: () => unknown = () => ({ +// Use globalThis to store the mock config so it survives vi.mock hoisting +const CONFIG_KEY = Symbol.for("warelay:testConfigMock"); +const DEFAULT_CONFIG = { inbound: { allowFrom: ["*"], // Allow all in tests by default messagePrefix: undefined, // No message prefix in tests responsePrefix: undefined, // No response prefix in tests timestampPrefix: false, // No timestamp in tests }, -}); +}; -export function setLoadConfigMock(fn: () => unknown) { - loadConfigMock = fn; +// Initialize default if not set +if (!(globalThis as Record)[CONFIG_KEY]) { + (globalThis as Record)[CONFIG_KEY] = () => DEFAULT_CONFIG; +} + +export function setLoadConfigMock(fn: (() => unknown) | unknown) { + (globalThis as Record)[CONFIG_KEY] = + typeof fn === "function" ? fn : () => fn; } export function resetLoadConfigMock() { - loadConfigMock = () => ({ - inbound: { - allowFrom: ["*"], - messagePrefix: undefined, - responsePrefix: undefined, - timestampPrefix: false, - }, - }); + (globalThis as Record)[CONFIG_KEY] = () => DEFAULT_CONFIG; } vi.mock("../config/config.js", () => ({ - loadConfig: () => loadConfigMock(), + loadConfig: () => { + const getter = (globalThis as Record)[CONFIG_KEY]; + if (typeof getter === "function") return getter(); + return DEFAULT_CONFIG; + }, })); vi.mock("../media/store.js", () => ({