mcp servers and ask followup are playing nicely. moved mcp server init to cli startup

Author: Eric Oliver

docs/technical/sliding-timeout-implementation.md

@@ -0,0 +1,101 @@
+# Sliding Timeout Implementation for Batch Execution
+
+## Problem
+
+The batch execution code had a fixed 60-second timeout that would kill processes regardless of whether work was happening. This was problematic for long-running tasks that had periods of activity.
+
+## Solution
+
+Implemented a sliding timeout that resets whenever there's activity, ensuring tasks only timeout during periods of inactivity.
+
+## Implementation
+
+### Key Changes in `src/cli/commands/batch.ts`
+
+1. **Replaced Fixed Timeout with Sliding Timeout**:
+
+    ```typescript
+    // Before: Fixed timeout
+    const timeout = setTimeout(() => {
+    	reject(new Error(`Task execution timeout after ${timeoutMs}ms`))
+    }, timeoutMs)
+
+    // After: Sliding timeout with reset capability
+    let timeout: NodeJS.Timeout
+    const resetTimeout = () => {
+    	if (timeout) clearTimeout(timeout)
+    	timeout = setTimeout(() => {
+    		reject(new Error(`Task execution timeout after ${timeoutMs}ms of inactivity`))
+    	}, timeoutMs)
+    }
+    ```
+
+2. **Activity Detection**:
+   The timeout resets on these Task events:
+
+    - `taskStarted` - When task begins
+    - `taskModeSwitched` - When switching between modes (e.g., code, debug)
+    - `taskPaused` / `taskUnpaused` - When task state changes
+    - `taskSpawned` - When new tasks are created
+    - `taskTokenUsageUpdated` - When API calls are made (frequent during active work)
+    - `message` - When messages are created/updated
+
+3. **User Input Handling**:
+
+    - Timeout is **paused** when questions are asked to the user
+    - Timeout is **resumed** when user responds (`taskAskResponded` event)
+    - This prevents false timeouts while waiting for user input
+    - Users can take as long as needed to respond to questions
+
+4. **Proper Cleanup**:
+    - Timeout is cleared when task completes successfully
+    - Timeout is cleared when task is aborted
+    - Timeout is cleared when tool failures occur
+
+## Benefits
+
+1. **No More False Timeouts**: Tasks won't timeout while actively working
+2. **Still Prevents Hanging**: Tasks will timeout after 60 seconds of true inactivity
+3. **Better User Experience**: Long-running tasks can complete without manual intervention
+4. **Maintains Safety**: Still protects against truly stuck processes
+
+## Testing
+
+Created comprehensive tests and a demonstration script showing:
+
+- Basic timeout behavior after inactivity period
+- Timeout reset on activity events
+- Proper cleanup on completion/abort
+- Multiple rapid activity events handling
+
+## Demo Output
+
+```
+=== Sliding Timeout Demo ===
+
+Scenario 1: Task times out after 5 seconds of inactivity
+Timeout reset - task has 5000ms of inactivity before timeout
+Task timed out after 5000ms of inactivity
+❌ Task timed out due to inactivity!
+
+Scenario 2: Task with periodic activity
+Timeout reset - task has 5000ms of inactivity before timeout
+Activity detected - resetting timeout
+Timeout reset - task has 5000ms of inactivity before timeout
+Activity detected - resetting timeout
+Timeout reset - task has 5000ms of inactivity before timeout
+
+Scenario 3: Task completes before timeout
+Timeout reset - task has 5000ms of inactivity before timeout
+Task completed - clearing timeout
+Timeout cleared
+✅ Task completed successfully before timeout
+```
+
+## Configuration
+
+The timeout duration remains configurable at 60 seconds (60000ms) but now represents inactivity timeout rather than total execution time.
+
+## Backward Compatibility
+
+This change is backward compatible - existing batch execution will work the same but with improved timeout behavior.

src/cli/commands/__tests__/BatchProcessor.test.ts

@@ -0,0 +1,255 @@
+import { BatchProcessor } from "../batch"
+import { Task } from "../../../core/task/Task"
+import { CliConfigManager } from "../../config/CliConfigManager"
+import { EventEmitter } from "events"
+
+// Mock dependencies
+jest.mock("../../../core/task/Task")
+jest.mock("../../config/CliConfigManager")
+jest.mock("../../services/CLILogger", () => ({
+	getCLILogger: () => ({
+		debug: jest.fn(),
+		info: jest.fn(),
+		error: jest.fn(),
+	}),
+}))
+jest.mock("../../../core/adapters/cli", () => ({
+	createCliAdapters: jest.fn().mockReturnValue({
+		fileSystem: {},
+		terminal: {},
+		browser: {},
+		telemetry: {},
+	}),
+}))
+jest.mock("../../services/CLIUIService")
+
+describe("BatchProcessor", () => {
+	let batchProcessor: BatchProcessor
+	let mockTask: any
+	let mockTaskPromise: Promise<void>
+	let mockConfigManager: any
+
+	const defaultOptions = {
+		cwd: "/test/cwd",
+		verbose: false,
+		color: false,
+	}
+
+	beforeEach(() => {
+		jest.clearAllMocks()
+		jest.useFakeTimers()
+
+		// Create a mock task that extends EventEmitter
+		mockTask = new EventEmitter()
+		mockTask.taskId = "test-task-id"
+		mockTask.isInitialized = true
+		mockTask.abort = false
+		mockTask.dispose = jest.fn().mockResolvedValue(undefined)
+
+		// Mock task promise that never resolves (we'll control completion via events)
+		mockTaskPromise = new Promise(() => {}) // Never resolves
+
+		// Mock Task.create to return our mock task and promise
+		;(Task.create as jest.Mock).mockReturnValue([mockTask, mockTaskPromise])
+
+		mockConfigManager = {
+			loadConfiguration: jest.fn().mockResolvedValue({
+				apiProvider: "anthropic",
+				apiKey: "test-key",
+				apiModelId: "claude-3-5-sonnet-20241022",
+			}),
+		}
+		;(CliConfigManager as any).mockImplementation(() => mockConfigManager)
+
+		batchProcessor = new BatchProcessor(defaultOptions, mockConfigManager)
+	})
+
+	afterEach(() => {
+		jest.useRealTimers()
+	})
+
+	describe("sliding timeout", () => {
+		it("should timeout after 60 seconds of inactivity", async () => {
+			const runPromise = batchProcessor.run("test task")
+
+			// Fast-forward 60 seconds
+			jest.advanceTimersByTime(60000)
+
+			await expect(runPromise).rejects.toThrow("Task execution timeout after 60000ms of inactivity")
+		})
+
+		it("should reset timeout when task activity occurs", async () => {
+			const runPromise = batchProcessor.run("test task")
+
+			// Wait 59 seconds (just before timeout)
+			jest.advanceTimersByTime(59000)
+
+			// Trigger activity event - this should reset the timeout
+			mockTask.emit("taskTokenUsageUpdated", "test-task-id", { inputTokens: 100 })
+
+			// Wait another 59 seconds (should not timeout yet)
+			jest.advanceTimersByTime(59000)
+
+			// Task should still be running, no timeout yet
+			expect(runPromise).not.toBe(undefined)
+
+			// Now wait the full 60 seconds from the last activity
+			jest.advanceTimersByTime(1000)
+
+			await expect(runPromise).rejects.toThrow("Task execution timeout after 60000ms of inactivity")
+		})
+
+		it("should reset timeout on various activity events", async () => {
+			const runPromise = batchProcessor.run("test task")
+
+			const activityEvents = [
+				["taskStarted"],
+				["taskModeSwitched", "test-task-id", "code"],
+				["taskPaused"],
+				["taskUnpaused"],
+				["taskAskResponded"],
+				["taskSpawned", "new-task-id"],
+				["taskTokenUsageUpdated", "test-task-id", { inputTokens: 100 }],
+				["message", { action: "created" }],
+			]
+
+			for (const event of activityEvents) {
+				// Wait 59 seconds
+				jest.advanceTimersByTime(59000)
+
+				// Trigger activity event
+				mockTask.emit(event[0], ...event.slice(1))
+
+				// Should not timeout yet
+				expect(runPromise).not.toBe(undefined)
+			}
+
+			// Wait full timeout after last activity
+			jest.advanceTimersByTime(60000)
+
+			await expect(runPromise).rejects.toThrow("Task execution timeout after 60000ms of inactivity")
+		})
+
+		it("should clear timeout when task completes successfully", async () => {
+			const runPromise = batchProcessor.run("test task")
+
+			// Wait 30 seconds
+			jest.advanceTimersByTime(30000)
+
+			// Complete the task
+			mockTask.emit("taskCompleted", "test-task-id", { inputTokens: 100 }, { toolName: 1 })
+
+			// Should resolve successfully
+			await expect(runPromise).resolves.toBeUndefined()
+
+			// Advance timers to ensure no timeout occurs
+			jest.advanceTimersByTime(120000)
+		})
+
+		it("should clear timeout when task is aborted", async () => {
+			const runPromise = batchProcessor.run("test task")
+
+			// Wait 30 seconds
+			jest.advanceTimersByTime(30000)
+
+			// Abort the task
+			mockTask.emit("taskAborted")
+
+			// Should reject with abort error
+			await expect(runPromise).rejects.toThrow("Task was aborted")
+
+			// Advance timers to ensure no timeout occurs
+			jest.advanceTimersByTime(120000)
+		})
+
+		it("should clear timeout when tool fails", async () => {
+			const runPromise = batchProcessor.run("test task")
+
+			// Wait 30 seconds
+			jest.advanceTimersByTime(30000)
+
+			// Trigger tool failure
+			mockTask.emit("taskToolFailed", "test-task-id", "read_file", "File not found")
+
+			// Should reject with tool failure error
+			await expect(runPromise).rejects.toThrow("Tool read_file failed: File not found")
+
+			// Advance timers to ensure no timeout occurs
+			jest.advanceTimersByTime(120000)
+		})
+
+		it("should handle multiple rapid activity events correctly", async () => {
+			const runPromise = batchProcessor.run("test task")
+
+			// Trigger multiple rapid events
+			for (let i = 0; i < 10; i++) {
+				jest.advanceTimersByTime(5000) // 5 seconds between events
+				mockTask.emit("taskTokenUsageUpdated", "test-task-id", { inputTokens: 100 + i })
+			}
+
+			// Total elapsed time: 50 seconds, but timeout should be reset to start from last event
+			// Wait another 59 seconds (should not timeout)
+			jest.advanceTimersByTime(59000)
+
+			// Should still be running
+			expect(runPromise).not.toBe(undefined)
+
+			// Wait the final second to trigger timeout
+			jest.advanceTimersByTime(1000)
+
+			await expect(runPromise).rejects.toThrow("Task execution timeout after 60000ms of inactivity")
+		})
+
+		it("should not reset timeout for non-activity events", async () => {
+			const runPromise = batchProcessor.run("test task")
+
+			// Wait 59 seconds
+			jest.advanceTimersByTime(59000)
+
+			// Trigger a non-activity event that shouldn't reset timeout
+			// (Note: In our current implementation, we don't have non-activity events that are listened to,
+			// but this test ensures we're only listening to the right events)
+
+			// Wait 1 more second to trigger timeout
+			jest.advanceTimersByTime(1000)
+
+			await expect(runPromise).rejects.toThrow("Task execution timeout after 60000ms of inactivity")
+		})
+	})
+
+	describe("error handling", () => {
+		it("should handle configuration loading errors gracefully", async () => {
+			mockConfigManager.loadConfiguration.mockRejectedValue(new Error("Config error"))
+
+			// Should fall back to environment variables
+			process.env.ANTHROPIC_API_KEY = "env-key"
+
+			const runPromise = batchProcessor.run("test task")
+
+			// Should still proceed with fallback config
+			expect(Task.create).toHaveBeenCalled()
+
+			// Clean up
+			delete process.env.ANTHROPIC_API_KEY
+
+			// Complete the task to avoid hanging test
+			mockTask.emit("taskCompleted", "test-task-id", {}, {})
+			await expect(runPromise).resolves.toBeUndefined()
+		})
+
+		it("should exit if no API key is available", async () => {
+			mockConfigManager.loadConfiguration.mockRejectedValue(new Error("Config error"))
+			delete process.env.ANTHROPIC_API_KEY
+			delete process.env.ROO_API_KEY
+
+			const mockExit = jest.spyOn(process, "exit").mockImplementation(() => {
+				throw new Error("process.exit called")
+			})
+
+			await expect(batchProcessor.run("test task")).rejects.toThrow("process.exit called")
+
+			expect(mockExit).toHaveBeenCalledWith(1)
+			mockExit.mockRestore()
+		})
+	})
+})