Harden worktree handling and queue routing

Author: htlin222

TASKS.md

@@ -0,0 +1,35 @@
+# Task List (Review Findings)
+
+Ordered by severity (High → Medium → Low). Source: recent changes review.
+
+## High
+
+- [x] Fix pending worktree intercepting all text (including commands) so users can still run /status, /stop, etc. (`src/handlers/text.ts:73`)
+- [x] Tie pending worktree to both userId + chatId to avoid cross-chat confusion. (`src/session.ts:276`)
+- [x] Add pending worktree expiry/timeout and clear it on /provider switch and session kill. (`src/session.ts:264`, `src/session.ts:295`, `src/session.ts:837`)
+- [x] Enforce allowlist validation when switching workingDir via worktree/branch/merge. (`src/handlers/text.ts:94`, `src/handlers/callback.ts:708`, `src/handlers/callback.ts:819`, `src/session.ts:320`)
+- [x] Add allowlist checks to sendfile callback to prevent arbitrary file exfiltration. (`src/handlers/callback.ts:732`)
+- [x] Add hard safety for Codex file operations to respect ALLOWED_PATHS (not just prompt). (`src/session.ts:638`, `src/providers/codex-worker.js:37`)
+
+## Medium
+
+- [x] Add timeout to git exec to avoid hangs. (`src/worktree.ts:43`)
+- [x] Guard `mkdirSync` in worktree creation with try/catch and user-friendly error. (`src/worktree.ts:444`)
+- [x] Reconcile worktree base directory with ALLOWED_PATHS (or auto-extend allowlist). (`src/worktree.ts:444`, `src/config.ts:102`, `src/security.ts:129`)
+- [x] Clear pending messages on session kill / worktree switch to avoid cross-context execution. (`src/session.ts:157`, `src/session.ts:837`)
+- [x] Support multiple pending worktree requests (at least scoped per chat) or reject with clearer feedback. (`src/session.ts:264`)
+- [x] Clear pending worktree state when switching via branch callback. (`src/handlers/callback.ts:708`, `src/session.ts:287`)
+- [x] Handle detached worktrees in worktree map to avoid wrong main path during merge. (`src/worktree.ts:89`, `src/worktree.ts:210`)
+- [x] Warn/block on dirty working tree before /merge. (`src/handlers/commands.ts:582`)
+- [x] Warn/block on dirty working tree before /worktree and /branch switches. (`src/handlers/commands.ts:499`, `src/handlers/commands.ts:538`)
+- [x] Provide user feedback when branches are omitted due to callback length. (`src/handlers/commands.ts:562`)
+- [x] Add paging/limit for long branch lists to avoid Telegram keyboard limits. (`src/handlers/commands.ts:562`)
+- [x] Add callback length check for /merge (long branch names). (`src/handlers/commands.ts:602`)
+
+## Low
+
+- [x] Add callback length check for /diff view (long file paths). (`src/handlers/commands.ts:1140`)
+- [x] Add file size limit + try/catch around diff file sending. (`src/handlers/callback.ts:916`)
+- [x] Handle spawn errors for Codex worker to avoid uncaught failures. (`src/providers/codex.ts:150`)
+- [x] Escape HTML for branch names in /branch and /merge messages. (`src/handlers/commands.ts:572`, `src/handlers/commands.ts:608`, `src/handlers/callback.ts:825`)
+- [x] Either wire `queryQueue` into handlers or remove dead code + tests. (`src/query-queue.ts:57`, `src/handlers/streaming.ts:281`)

src/__tests__/logger.test.ts

@@ -19,6 +19,14 @@ describe("Logger", () => {
 		console.error = consoleErrorMock;
 	});
 
+	const getCall = (mockFn: ReturnType<typeof mock>, index = 0): string => {
+		const call = mockFn.mock.calls[index]?.[0];
+		if (call === undefined) {
+			throw new Error("Expected console call");
+		}
+		return String(call);
+	};
+
 	afterEach(() => {
 		consoleLogMock.mockRestore?.();
 		consoleWarnMock.mockRestore?.();
@@ -98,7 +106,7 @@ describe("Logger", () => {
 			const log = new Logger("info");
 			log.info("test message");
 
-			const call = consoleLogMock.mock.calls[0][0] as string;
+			const call = getCall(consoleLogMock);
 			// Check for ISO timestamp pattern: [YYYY-MM-DDTHH:MM:SS.sssZ]
 			expect(call).toMatch(/^\[\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}.\d{3}Z\]/);
 		});
@@ -107,23 +115,23 @@ describe("Logger", () => {
 			const log = new Logger("debug");
 
 			log.debug("test");
-			expect(consoleLogMock.mock.calls[0][0]).toContain("DEBUG");
+			expect(getCall(consoleLogMock, 0)).toContain("DEBUG");
 
 			log.info("test");
-			expect(consoleLogMock.mock.calls[1][0]).toContain("INFO");
+			expect(getCall(consoleLogMock, 1)).toContain("INFO");
 
 			log.warn("test");
-			expect(consoleWarnMock.mock.calls[0][0]).toContain("WARN");
+			expect(getCall(consoleWarnMock, 0)).toContain("WARN");
 
 			log.error("test");
-			expect(consoleErrorMock.mock.calls[0][0]).toContain("ERROR");
+			expect(getCall(consoleErrorMock, 0)).toContain("ERROR");
 		});
 
 		test("includes the message", () => {
 			const log = new Logger("info");
 			log.info("Hello, world!");
 
-			const call = consoleLogMock.mock.calls[0][0] as string;
+			const call = getCall(consoleLogMock);
 			expect(call).toContain("Hello, world!");
 		});
 	});
@@ -133,23 +141,23 @@ describe("Logger", () => {
 			const log = new Logger("info");
 			log.info("Request received", { method: "GET", path: "/api/users" });
 
-			const call = consoleLogMock.mock.calls[0][0] as string;
+			const call = getCall(consoleLogMock);
 			expect(call).toContain('{"method":"GET","path":"/api/users"}');
 		});
 
 		test("handles empty context object", () => {
 			const log = new Logger("info");
 			log.info("Simple message", {});
 
-			const call = consoleLogMock.mock.calls[0][0] as string;
+			const call = getCall(consoleLogMock);
 			expect(call).not.toContain("{}");
 		});
 
 		test("handles undefined context", () => {
 			const log = new Logger("info");
 			log.info("Simple message");
 
-			const call = consoleLogMock.mock.calls[0][0] as string;
+			const call = getCall(consoleLogMock);
 			expect(call).toContain("Simple message");
 			// Should not have trailing JSON
 			expect(call).toMatch(/Simple message$/);
@@ -162,7 +170,7 @@ describe("Logger", () => {
 				tags: ["admin", "active"],
 			});
 
-			const call = consoleLogMock.mock.calls[0][0] as string;
+			const call = getCall(consoleLogMock);
 			expect(call).toContain('"user":{"id":123,"name":"Alice"}');
 			expect(call).toContain('"tags":["admin","active"]');
 		});
@@ -171,7 +179,7 @@ describe("Logger", () => {
 			const log = new Logger("info");
 			log.info("Status", { count: 42, enabled: true, ratio: 3.14 });
 
-			const call = consoleLogMock.mock.calls[0][0] as string;
+			const call = getCall(consoleLogMock);
 			expect(call).toContain('"count":42');
 			expect(call).toContain('"enabled":true');
 			expect(call).toContain('"ratio":3.14');

src/config.ts

@@ -5,7 +5,7 @@
  */
 
 import { homedir } from "node:os";
-import { dirname, resolve } from "node:path";
+import { basename, dirname, resolve } from "node:path";
 import type { McpServerConfig } from "./types";
 
 // ============== Environment Setup ==============
@@ -100,8 +100,10 @@ export { MCP_SERVERS };
 // ============== Security Configuration ==============
 
 // Allowed directories for file operations
+const WORKTREE_BASE_DIR = resolve(WORKING_DIR, "..", "worktree");
 const defaultAllowedPaths = [
 	WORKING_DIR,
+	...(basename(WORKTREE_BASE_DIR) === "worktree" ? [WORKTREE_BASE_DIR] : []),
 	`${HOME}/Documents`,
 	`${HOME}/Downloads`,
 	`${HOME}/Desktop`,

src/handlers/callback.ts

@@ -6,14 +6,16 @@
 
 import { unlinkSync } from "node:fs";
 import { type Context, InlineKeyboard } from "grammy";
-import { addBookmark, removeBookmark } from "../bookmarks";
+import { addBookmark, removeBookmark, resolvePath } from "../bookmarks";
 import {
 	AGENT_PROVIDERS,
 	ALLOWED_USERS,
 	type AgentProviderId,
 	MESSAGE_EFFECTS,
 } from "../config";
-import { isAuthorized } from "../security";
+import { escapeHtml } from "../formatting";
+import { queryQueue } from "../query-queue";
+import { isAuthorized, isPathAllowed } from "../security";
 import { session } from "../session";
 import { auditLog, startTypingIndicator } from "../utils";
 import { logNonCriticalError } from "../utils/error-logging";
@@ -97,7 +99,7 @@ export async function handleCallback(ctx: Context): Promise<void> {
 
 	// 2h. Handle branch callbacks
 	if (callbackData.startsWith("branch:")) {
-		await handleBranchCallback(ctx, callbackData);
+		await handleBranchCallback(ctx, userId, chatId, callbackData);
 		return;
 	}
 
@@ -197,7 +199,7 @@ export async function handleCallback(ctx: Context): Promise<void> {
 	const statusCallback = createStatusCallback(ctx, state);
 
 	try {
-		const response = await session.sendMessageStreaming(
+		const response = await queryQueue.sendMessage(
 			message,
 			username,
 			userId,
@@ -372,7 +374,7 @@ async function handlePendingCallback(
 		const statusCallback = createStatusCallback(ctx, state);
 
 		try {
-			const response = await session.sendMessageStreaming(
+			const response = await queryQueue.sendMessage(
 				message,
 				username,
 				userId,
@@ -433,7 +435,7 @@ async function handleActionCallback(
 	const statusCallback = createStatusCallback(ctx, state);
 
 	try {
-		const response = await session.sendMessageStreaming(
+		const response = await queryQueue.sendMessage(
 			command,
 			username,
 			userId,
@@ -672,6 +674,8 @@ async function handleProviderCallback(
  */
 async function handleBranchCallback(
 	ctx: Context,
+	userId: number,
+	chatId: number,
 	callbackData: string,
 ): Promise<void> {
 	const prefix = "branch:switch:";
@@ -705,19 +709,35 @@ async function handleBranchCallback(
 		return;
 	}
 
+	if (!isPathAllowed(result.path)) {
+		await ctx.answerCallbackQuery({
+			text: "Worktree path is not in allowed directories.",
+		});
+		try {
+			await ctx.reply(
+				`❌ Worktree path is not in allowed directories:\n<code>${escapeHtml(result.path)}</code>\n\nUpdate ALLOWED_PATHS and try again.`,
+				{ parse_mode: "HTML" },
+			);
+		} catch (error) {
+			logNonCriticalError("branch allowlist reply", error);
+		}
+		return;
+	}
+
 	// Save current session before switching
 	session.flushSession();
 	session.setWorkingDir(result.path);
 	await session.kill();
+	session.clearWorktreeRequest(userId, chatId);
 
 	try {
 		await ctx.editMessageText(
-			`✅ Switched to worktree:\n<code>${result.path}</code>\n\nBranch: <code>${result.branch}</code>`,
+			`✅ Switched to worktree:\n<code>${escapeHtml(result.path)}</code>\n\nBranch: <code>${escapeHtml(result.branch)}</code>`,
 			{ parse_mode: "HTML" },
 		);
 	} catch {
 		await ctx.reply(
-			`✅ Switched to worktree:\n<code>${result.path}</code>\n\nBranch: <code>${result.branch}</code>`,
+			`✅ Switched to worktree:\n<code>${escapeHtml(result.path)}</code>\n\nBranch: <code>${escapeHtml(result.branch)}</code>`,
 			{ parse_mode: "HTML" },
 		);
 	}
@@ -747,17 +767,28 @@ async function handleSendFileCallback(
 		return;
 	}
 
+	const resolvedPath = resolvePath(filePath, session.workingDir);
+
 	// Check file exists
-	if (!existsSync(filePath)) {
+	if (!existsSync(resolvedPath)) {
 		await ctx.answerCallbackQuery({ text: "File not found" });
 		return;
 	}
 
+	if (!isPathAllowed(resolvedPath)) {
+		await ctx.answerCallbackQuery({ text: "Access denied" });
+		await ctx.reply(
+			`❌ Access denied: <code>${escapeHtml(resolvedPath)}</code>`,
+			{ parse_mode: "HTML" },
+		);
+		return;
+	}
+
 	// Send the file
 	try {
 		await ctx.answerCallbackQuery({ text: "Sending file..." });
-		const fileName = basename(filePath);
-		await ctx.replyWithDocument(new InputFile(filePath, fileName));
+		const fileName = basename(resolvedPath);
+		await ctx.replyWithDocument(new InputFile(resolvedPath, fileName));
 	} catch (error) {
 		console.error("Failed to send file:", error);
 		await ctx.reply(`❌ Failed to send file: ${String(error).slice(0, 100)}`);
@@ -816,14 +847,25 @@ async function handleMergeCallback(
 		return;
 	}
 
+	if (!isPathAllowed(info.mainWorktreePath)) {
+		await ctx.answerCallbackQuery({
+			text: "Main worktree path is not in allowed directories.",
+		});
+		await ctx.reply(
+			`❌ Main worktree path is not in allowed directories:\n<code>${escapeHtml(info.mainWorktreePath)}</code>\n\nUpdate ALLOWED_PATHS and try again.`,
+			{ parse_mode: "HTML" },
+		);
+		return;
+	}
+
 	// Switch to main worktree
 	session.flushSession();
 	session.setWorkingDir(info.mainWorktreePath);
 	await session.kill();
 
 	try {
 		await ctx.editMessageText(
-			`🔀 Switched to <code>${info.mainBranch}</code> worktree.\n\nMerging <code>${branchToMerge}</code>...`,
+			`🔀 Switched to <code>${escapeHtml(info.mainBranch)}</code> worktree.\n\nMerging <code>${escapeHtml(branchToMerge)}</code>...`,
 			{ parse_mode: "HTML" },
 		);
 	} catch (error) {
@@ -849,7 +891,7 @@ If the merge is clean, just complete it. If there are conflicts, explain what yo
 	const chatId = ctx.chat?.id;
 
 	try {
-		const response = await session.sendMessageStreaming(
+		const response = await queryQueue.sendMessage(
 			mergePrompt,
 			username,
 			userId,
@@ -919,10 +961,23 @@ async function handleDiffCallback(
 
 			const { InputFile } = await import("grammy");
 			const diffBuffer = Buffer.from(result.fullDiff, "utf-8");
+			const MAX_DIFF_SIZE = 50 * 1024 * 1024;
+			if (diffBuffer.length > MAX_DIFF_SIZE) {
+				await ctx.answerCallbackQuery({ text: "Diff file too large" });
+				await ctx.reply("❌ Diff is too large to send as a file.");
+				return;
+			}
 			const filename = file
 				? `${file.replace(/\//g, "_")}.diff`
 				: "changes.diff";
-			await ctx.replyWithDocument(new InputFile(diffBuffer, filename));
+			try {
+				await ctx.replyWithDocument(new InputFile(diffBuffer, filename));
+			} catch (error) {
+				console.error("Failed to send diff file:", error);
+				await ctx.reply(
+					`❌ Failed to send diff file: ${String(error).slice(0, 100)}`,
+				);
+			}
 		} else {
 			// Send as HTML pre block
 			await ctx.answerCallbackQuery({ text: "Showing diff..." });
@@ -961,7 +1016,7 @@ async function handleDiffCallback(
 		const chatId = ctx.chat?.id;
 
 		try {
-			const response = await session.sendMessageStreaming(
+			const response = await queryQueue.sendMessage(
 				"/commit",
 				username,
 				userId,