diff --git a/src-tauri/src/wsl_bridge.rs b/src-tauri/src/wsl_bridge.rs index c2da718..945a11d 100644 --- a/src-tauri/src/wsl_bridge.rs +++ b/src-tauri/src/wsl_bridge.rs @@ -1,9 +1,9 @@ use std::io::{BufRead, BufReader, Write}; use std::process::{Child, ChildStdin, Command, Stdio}; -use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; use std::sync::Arc; use std::thread; -use std::time::{Duration, SystemTime, UNIX_EPOCH}; +use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; use parking_lot::Mutex; use tauri::{AppHandle, Emitter}; use tempfile::NamedTempFile; @@ -114,6 +114,11 @@ pub struct WslBridge { /// Set to true by stop()/interrupt() before killing the process so handle_stdout knows /// the disconnect was intentional and should not emit a second Disconnected event. intentional_stop: Arc, + /// Tracks when the most recent user message was sent. Cleared when a `Result` message + /// arrives. The mid-session watchdog uses this to detect a stuck process. + pending_since: Arc>>, + /// Incremented each time `start()` is called so each session's watchdog knows when to exit. + watchdog_generation: Arc, } impl WslBridge { @@ -128,6 +133,8 @@ impl WslBridge { conversation_id: None, received_init: Arc::new(AtomicBool::new(false)), intentional_stop: Arc::new(AtomicBool::new(false)), + pending_since: Arc::new(Mutex::new(None)), + watchdog_generation: Arc::new(AtomicU64::new(0)), } } @@ -142,6 +149,8 @@ impl WslBridge { conversation_id: Some(conversation_id), received_init: Arc::new(AtomicBool::new(false)), intentional_stop: Arc::new(AtomicBool::new(false)), + pending_since: Arc::new(Mutex::new(None)), + watchdog_generation: Arc::new(AtomicU64::new(0)), } } @@ -442,6 +451,7 @@ impl WslBridge { let conv_id = self.conversation_id.clone(); let received_init_clone = self.received_init.clone(); let intentional_stop_clone = self.intentional_stop.clone(); + let pending_since_clone = self.pending_since.clone(); thread::spawn(move || { handle_stdout( stdout, @@ -450,6 +460,7 @@ impl WslBridge { conv_id, received_init_clone, intentional_stop_clone, + pending_since_clone, ); }); } @@ -487,6 +498,48 @@ impl WslBridge { } }); + // Reset the pending-since tracker for this new session so stale state from a previous + // session never triggers the mid-session watchdog immediately. + *self.pending_since.lock() = None; + + // Mid-session watchdog: if a user message is sent but no Result arrives within 5 minutes, + // the Claude Code process is stuck. Kill it so the user gets a disconnect event and can + // reconnect. The generation counter ensures old watchdogs from previous sessions exit + // cleanly when `start()` is called again. + let generation = self.watchdog_generation.fetch_add(1, Ordering::SeqCst) + 1; + let process_mid_watchdog = self.process.clone(); + let pending_since_watchdog = self.pending_since.clone(); + let generation_watchdog = self.watchdog_generation.clone(); + const STUCK_TIMEOUT: Duration = Duration::from_secs(5 * 60); + const POLL_INTERVAL: Duration = Duration::from_secs(30); + thread::spawn(move || { + loop { + thread::sleep(POLL_INTERVAL); + // Exit if a newer session has started. + if generation_watchdog.load(Ordering::SeqCst) != generation { + break; + } + // Exit if the process has already been taken (killed or stopped). + if process_mid_watchdog.lock().is_none() { + break; + } + let elapsed = (*pending_since_watchdog.lock()).map(|t| t.elapsed()); + if let Some(elapsed) = elapsed { + if elapsed >= STUCK_TIMEOUT { + tracing::warn!( + "Mid-session watchdog: no Result received in {:?}; killing stuck process", + elapsed + ); + if let Some(mut proc) = process_mid_watchdog.lock().take() { + let _ = proc.kill(); + let _ = proc.wait(); + } + break; + } + } + } + }); + Ok(()) } @@ -523,6 +576,10 @@ impl WslBridge { .flush() .map_err(|e| format!("Failed to flush stdin: {}", e))?; + // Record the time this message was sent so the mid-session watchdog can detect + // if no Result ever arrives (i.e. the process is stuck). + *self.pending_since.lock() = Some(Instant::now()); + Ok(()) } @@ -762,15 +819,21 @@ fn handle_stdout( conversation_id: Option, received_init: Arc, intentional_stop: Arc, + pending_since: Arc>>, ) { let reader = BufReader::new(stdout); for line in reader.lines() { match line { Ok(line) if !line.is_empty() => { - if let Err(e) = - process_json_line(&line, &app, &stats, &conversation_id, &received_init) - { + if let Err(e) = process_json_line( + &line, + &app, + &stats, + &conversation_id, + &received_init, + &pending_since, + ) { tracing::error!("Error processing line: {}", e); } } @@ -1026,6 +1089,7 @@ fn process_json_line( stats: &Arc>, conversation_id: &Option, received_init: &Arc, + pending_since: &Arc>>, ) -> Result<(), String> { let message: ClaudeMessage = serde_json::from_str(line) .map_err(|e| format!("Failed to parse JSON: {} - Line: {}", e, line))?; @@ -1412,6 +1476,10 @@ fn process_json_line( permission_denials.as_ref().map(|d| d.len()) ); + // A Result means the turn is complete — clear pending_since so the mid-session + // watchdog knows the process is not stuck. + *pending_since.lock() = None; + let state = if subtype == "success" { CharacterState::Success } else { @@ -2493,4 +2561,98 @@ mod tests { panic!("Expected ToolResult variant"); } } + + // Mid-session watchdog: pending_since lifecycle tests + + #[test] + fn test_pending_since_starts_as_none() { + let bridge = WslBridge::new(); + assert!(bridge.pending_since.lock().is_none()); + } + + #[test] + fn test_watchdog_generation_starts_at_zero() { + let bridge = WslBridge::new(); + assert_eq!(bridge.watchdog_generation.load(Ordering::SeqCst), 0); + } + + #[test] + fn test_pending_since_set_reflects_elapsed_time() { + let pending_since: Arc>> = Arc::new(Mutex::new(None)); + + // Simulate send_message setting pending_since + *pending_since.lock() = Some(Instant::now()); + + // Should be Some and elapsed should be tiny (< 1 second) + let elapsed = (*pending_since.lock()).map(|t| t.elapsed()); + assert!(elapsed.is_some()); + assert!(elapsed.unwrap() < Duration::from_secs(1)); + } + + #[test] + fn test_pending_since_cleared_on_result_simulates_watchdog_safe() { + let pending_since: Arc>> = Arc::new(Mutex::new(None)); + + // Simulate send_message + *pending_since.lock() = Some(Instant::now()); + assert!(pending_since.lock().is_some()); + + // Simulate Result message arriving (as process_json_line does) + *pending_since.lock() = None; + assert!(pending_since.lock().is_none()); + + // Watchdog check: elapsed is None → no kill triggered + let elapsed = (*pending_since.lock()).map(|t| t.elapsed()); + assert!(elapsed.is_none()); + } + + #[test] + fn test_watchdog_generation_increments_per_session() { + let bridge = WslBridge::new(); + assert_eq!(bridge.watchdog_generation.load(Ordering::SeqCst), 0); + + // Simulate what start() does: fetch_add(1) returns old value, +1 gives new generation + let gen1 = bridge.watchdog_generation.fetch_add(1, Ordering::SeqCst) + 1; + assert_eq!(gen1, 1); + assert_eq!(bridge.watchdog_generation.load(Ordering::SeqCst), 1); + + let gen2 = bridge.watchdog_generation.fetch_add(1, Ordering::SeqCst) + 1; + assert_eq!(gen2, 2); + assert_eq!(bridge.watchdog_generation.load(Ordering::SeqCst), 2); + } + + #[test] + fn test_watchdog_generation_mismatch_means_old_session() { + // Simulate: watchdog captured generation=1, but start() was called again → generation=2. + // The watchdog should detect this and exit without killing. + let generation_arc: Arc = Arc::new(AtomicU64::new(1)); + let captured_generation: u64 = 1; + + assert_eq!(generation_arc.load(Ordering::SeqCst), captured_generation, "same session"); + + // New start() call increments generation + generation_arc.fetch_add(1, Ordering::SeqCst); + + assert_ne!( + generation_arc.load(Ordering::SeqCst), + captured_generation, + "old watchdog detects new session and should exit" + ); + } + + #[test] + fn test_stuck_timeout_threshold() { + // The timeout constant used in the mid-session watchdog is 5 minutes. + // This test documents and validates that threshold. + const STUCK_TIMEOUT: Duration = Duration::from_secs(5 * 60); + assert_eq!(STUCK_TIMEOUT, Duration::from_secs(300)); + + // A message sent 4m59s ago should NOT trigger the watchdog + let just_under = Duration::from_secs(299); + assert!(just_under < STUCK_TIMEOUT); + + // A message sent 5m0s ago SHOULD trigger the watchdog + let exactly_at = Duration::from_secs(300); + assert!(exactly_at >= STUCK_TIMEOUT); + } } diff --git a/src/lib/components/InputBar.svelte b/src/lib/components/InputBar.svelte index 024aedc..b682753 100644 --- a/src/lib/components/InputBar.svelte +++ b/src/lib/components/InputBar.svelte @@ -248,7 +248,7 @@ const hasAttachments = attachments.length > 0; // Need either a message or attachments to submit - if ((!message && !hasAttachments) || isSubmitting) return; + if ((!message && !hasAttachments) || isSubmitting || isProcessing) return; // Check for slash commands first (these work even when disconnected) if (message && isSlashCommand(message)) { @@ -339,6 +339,7 @@ User: ${formattedMessage}`; conversationId, message: messageToSend, }); + claudeStore.setProcessing(true); } catch (error) { console.error("Failed to send prompt:", error); claudeStore.addLine("error", `Failed to send: ${error}`); @@ -768,7 +769,7 @@ User: ${formattedMessage}`; async function handleQuickAction(prompt: string): Promise { // Quick actions send the prompt directly - if (!isConnected || isSubmitting) return; + if (!isConnected || isSubmitting || isProcessing) return; // Add to history addToHistory(prompt); @@ -793,6 +794,7 @@ User: ${formattedMessage}`; conversationId, message: prompt, }); + claudeStore.setProcessing(true); } catch (error) { console.error("Failed to send quick action:", error); claudeStore.addLine("error", `Failed to send: ${error}`); @@ -1018,7 +1020,7 @@ User: ${formattedMessage}`; placeholder={isConnected ? "Ask Hikari anything... (type / for commands)" : "Connect to Claude first..."} - disabled={isSubmitting} + disabled={isSubmitting || isProcessing} rows={1} style="height: {textareaHeight}px; font-size: var(--terminal-font-size, 14px); font-family: var(--terminal-font-family, monospace);" class="w-full px-4 py-3 bg-[var(--bg-secondary)] border border-[var(--border-color)] diff --git a/src/lib/components/PermissionModal.svelte b/src/lib/components/PermissionModal.svelte index 061e4b9..0b75bf3 100644 --- a/src/lib/components/PermissionModal.svelte +++ b/src/lib/components/PermissionModal.svelte @@ -86,7 +86,7 @@ api_key: config.api_key || null, custom_instructions: config.custom_instructions || null, mcp_servers_json: config.mcp_servers_json || null, - allowed_tools: newGrantedTools, + allowed_tools: [...new Set([...newGrantedTools, ...config.auto_granted_tools])], use_worktree: config.use_worktree ?? false, disable_1m_context: config.disable_1m_context ?? false, }, diff --git a/src/lib/components/PermissionModal.test.ts b/src/lib/components/PermissionModal.test.ts new file mode 100644 index 0000000..8eb4d27 --- /dev/null +++ b/src/lib/components/PermissionModal.test.ts @@ -0,0 +1,154 @@ +/** + * PermissionModal Component Tests + * + * Tests the pure helper functions used by the PermissionModal component. + * + * What this component does: + * - Displays pending permission requests from Claude Code + * - Allows the user to approve or dismiss permission requests + * - On approval, reconnects Claude with the newly granted tools merged with + * `auto_granted_tools` from config (bug fix: issue #198) + * - Restores conversation context after reconnecting + * + * Manual testing checklist: + * - [ ] Permission modal appears when Claude requests a tool not in allowed_tools + * - [ ] All permissions are pre-selected by default when modal opens + * - [ ] "Select All" and "Select None" buttons work correctly + * - [ ] "Already Granted" badge appears for tools already in the session grant list + * - [ ] Approving permissions reconnects Claude and restores conversation context + * - [ ] After reconnecting, auto_granted_tools are still respected (no re-prompting) + * - [ ] Dismissing the modal clears pending permissions without reconnecting + * - [ ] Enter key approves selected permissions + * - [ ] Escape key dismisses the modal + * - [ ] Character enters "permission" state when modal appears + * - [ ] Input details are shown in a collapsible "View details" section + * + * Note: The `handleApproveAndReconnect` function cannot be unit tested here + * because it depends on Tauri IPC calls (`invoke("stop_claude")`, + * `invoke("start_claude")`, `invoke("send_prompt")`). The critical bug fix + * (including `auto_granted_tools` in the reconnect's `allowed_tools`) is + * covered by the `buildAllowedToolsList` tests below, which replicate the + * exact merging logic from the component. + */ + +import { describe, it, expect } from "vitest"; + +/** + * Replicates the allowed-tools merging logic from PermissionModal's + * handleApproveAndReconnect. This is the fix for issue #198: previously, + * `auto_granted_tools` were not included when reconnecting, causing them to + * be silently dropped and prompting the user again on subsequent requests. + */ +function buildAllowedToolsList( + sessionGrantedTools: string[], + newlyGrantedTools: string[], + autoGrantedTools: string[] +): string[] { + return [...new Set([...sessionGrantedTools, ...newlyGrantedTools, ...autoGrantedTools])]; +} + +/** + * Replicates the formatInput helper from PermissionModal, used to display + * the tool input JSON in the permission details section. + */ +function formatInput(input: Record): string { + try { + return JSON.stringify(input, null, 2); + } catch { + return String(input); + } +} + +/** + * Replicates the isToolAlreadyGranted helper from PermissionModal. + */ +function isToolAlreadyGranted(toolName: string, grantedToolsList: string[]): boolean { + return grantedToolsList.includes(toolName); +} + +// --- + +describe("buildAllowedToolsList", () => { + it("merges session-granted, newly-granted, and auto-granted tools", () => { + const result = buildAllowedToolsList(["Bash"], ["Glob"], ["Read"]); + expect(result).toContain("Bash"); + expect(result).toContain("Glob"); + expect(result).toContain("Read"); + }); + + it("deduplicates tools that appear in multiple lists", () => { + const result = buildAllowedToolsList(["Read", "Bash"], ["Read"], ["Read", "Write"]); + const readCount = result.filter((t) => t === "Read").length; + expect(readCount).toBe(1); + }); + + it("preserves auto_granted_tools even when session list is empty", () => { + const result = buildAllowedToolsList([], ["Bash"], ["Read", "Glob"]); + expect(result).toContain("Read"); + expect(result).toContain("Glob"); + expect(result).toContain("Bash"); + }); + + it("returns only auto_granted_tools when no other grants exist", () => { + const result = buildAllowedToolsList([], [], ["Read"]); + expect(result).toEqual(["Read"]); + }); + + it("returns an empty array when all lists are empty", () => { + const result = buildAllowedToolsList([], [], []); + expect(result).toEqual([]); + }); + + it("reproduces the bug scenario from issue #198", () => { + // Scenario: user has Read in auto_granted_tools. + // Session starts correctly with Read allowed. + // User approves Bash via permission modal. + // Before fix: reconnect only passed [Bash], dropping Read. + // After fix: reconnect passes [Bash, Read]. + const sessionGrantedTools: string[] = []; // no prior session grants + const newlyGrantedTools = ["Bash"]; // just approved via modal + const autoGrantedTools = ["Read"]; // configured default + + const result = buildAllowedToolsList(sessionGrantedTools, newlyGrantedTools, autoGrantedTools); + + expect(result).toContain("Bash"); + expect(result).toContain("Read"); // Must be present — this was the bug! + }); +}); + +describe("formatInput", () => { + it("formats a simple object as pretty-printed JSON", () => { + const result = formatInput({ file_path: "/home/naomi/test.ts" }); + expect(result).toBe(JSON.stringify({ file_path: "/home/naomi/test.ts" }, null, 2)); + }); + + it("formats a nested object correctly", () => { + const input = { command: "ls", args: ["-la", "/home"] }; + const result = formatInput(input); + expect(result).toContain('"command": "ls"'); + expect(result).toContain('"args"'); + }); + + it("formats an empty object as '{}'", () => { + const result = formatInput({}); + expect(result).toBe("{}"); + }); +}); + +describe("isToolAlreadyGranted", () => { + it("returns true when the tool is in the granted list", () => { + expect(isToolAlreadyGranted("Read", ["Read", "Bash"])).toBe(true); + }); + + it("returns false when the tool is not in the granted list", () => { + expect(isToolAlreadyGranted("Write", ["Read", "Bash"])).toBe(false); + }); + + it("returns false for an empty granted list", () => { + expect(isToolAlreadyGranted("Read", [])).toBe(false); + }); + + it("is case-sensitive", () => { + expect(isToolAlreadyGranted("read", ["Read"])).toBe(false); + }); +}); diff --git a/src/lib/stores/claude.ts b/src/lib/stores/claude.ts index 8220beb..b163f73 100644 --- a/src/lib/stores/claude.ts +++ b/src/lib/stores/claude.ts @@ -41,6 +41,7 @@ export const claudeStore = { setWorkingDirectory: conversationsStore.setWorkingDirectory, setWorkingDirectoryForConversation: conversationsStore.setWorkingDirectoryForConversation, setProcessing: conversationsStore.setProcessing, + setProcessingForConversation: conversationsStore.setProcessingForConversation, addLine: conversationsStore.addLine, addLineToConversation: conversationsStore.addLineToConversation, updateLine: conversationsStore.updateLine, diff --git a/src/lib/stores/conversations.test.ts b/src/lib/stores/conversations.test.ts index c587109..2a5d9e7 100644 --- a/src/lib/stores/conversations.test.ts +++ b/src/lib/stores/conversations.test.ts @@ -561,3 +561,99 @@ describe("draft text persistence", () => { expect(conversation.draftText).toBe(""); }); }); + +describe("isProcessing state management", () => { + it("starts as false by default", () => { + const conversation = { id: "conv-1", isProcessing: false }; + expect(conversation.isProcessing).toBe(false); + }); + + it("setProcessingForConversation sets processing true for the target conversation", () => { + const conversations = new Map([ + ["conv-1", { isProcessing: false, lastActivityAt: new Date(0) }], + ["conv-2", { isProcessing: false, lastActivityAt: new Date(0) }], + ]); + + const setProcessingForConversation = (conversationId: string, processing: boolean) => { + const conv = conversations.get(conversationId); + if (conv) { + conv.isProcessing = processing; + conv.lastActivityAt = new Date(); + } + }; + + setProcessingForConversation("conv-1", true); + + expect(conversations.get("conv-1")?.isProcessing).toBe(true); + expect(conversations.get("conv-2")?.isProcessing).toBe(false); + }); + + it("setProcessingForConversation resets processing to false", () => { + const conversations = new Map([ + ["conv-1", { isProcessing: true, lastActivityAt: new Date(0) }], + ]); + + const setProcessingForConversation = (conversationId: string, processing: boolean) => { + const conv = conversations.get(conversationId); + if (conv) { + conv.isProcessing = processing; + conv.lastActivityAt = new Date(); + } + }; + + setProcessingForConversation("conv-1", false); + + expect(conversations.get("conv-1")?.isProcessing).toBe(false); + }); + + it("setProcessingForConversation does nothing for unknown conversation", () => { + const conversations = new Map([ + ["conv-1", { isProcessing: false, lastActivityAt: new Date(0) }], + ]); + + const setProcessingForConversation = (conversationId: string, processing: boolean) => { + const conv = conversations.get(conversationId); + if (conv) { + conv.isProcessing = processing; + conv.lastActivityAt = new Date(); + } + }; + + setProcessingForConversation("unknown", true); + + expect(conversations.get("conv-1")?.isProcessing).toBe(false); + }); + + it("isProcessing is cleared when idle state arrives", () => { + const conversation = { isProcessing: true, characterState: "thinking" }; + + const terminalStates = ["idle", "success", "error"]; + const handleStateChange = (state: string) => { + conversation.characterState = state; + if (terminalStates.includes(state)) { + conversation.isProcessing = false; + } + }; + + handleStateChange("idle"); + + expect(conversation.isProcessing).toBe(false); + }); + + it("isProcessing stays true during non-terminal states", () => { + const conversation = { isProcessing: true, characterState: "thinking" }; + + const terminalStates = ["idle", "success", "error"]; + const handleStateChange = (state: string) => { + conversation.characterState = state; + if (terminalStates.includes(state)) { + conversation.isProcessing = false; + } + }; + + for (const state of ["thinking", "typing", "coding", "searching", "mcp"]) { + handleStateChange(state); + expect(conversation.isProcessing).toBe(true); + } + }); +}); diff --git a/src/lib/stores/conversations.ts b/src/lib/stores/conversations.ts index a3968b4..666b4d1 100644 --- a/src/lib/stores/conversations.ts +++ b/src/lib/stores/conversations.ts @@ -560,6 +560,17 @@ function createConversationsStore() { }); }, + setProcessingForConversation: (conversationId: string, processing: boolean) => { + conversations.update((convs) => { + const conv = convs.get(conversationId); + if (conv) { + conv.isProcessing = processing; + conv.lastActivityAt = new Date(); + } + return convs; + }); + }, + addLine: ( type: TerminalLine["type"], content: string, diff --git a/src/lib/tauri.ts b/src/lib/tauri.ts index 98374c1..274d7f1 100644 --- a/src/lib/tauri.ts +++ b/src/lib/tauri.ts @@ -236,9 +236,10 @@ export async function initializeTauriListeners() { ); } - // Update character state for this conversation + // Update character state and processing state for this conversation if (targetConversationId) { claudeStore.setCharacterStateForConversation(targetConversationId, "idle"); + claudeStore.setProcessingForConversation(targetConversationId, false); } } else if (status === "error") { const targetConversationId = conversation_id || get(claudeStore.activeConversationId); @@ -333,13 +334,21 @@ export async function initializeTauriListeners() { } // Always update the conversation's state + const isTerminalState = + mappedState === "idle" || mappedState === "success" || mappedState === "error"; if (conversation_id) { claudeStore.setCharacterStateForConversation(conversation_id, mappedState); + if (isTerminalState) { + claudeStore.setProcessingForConversation(conversation_id, false); + } } else { // Fallback to active conversation if no conversation_id const activeConversationId = get(claudeStore.activeConversationId); if (activeConversationId) { claudeStore.setCharacterStateForConversation(activeConversationId, mappedState); + if (isTerminalState) { + claudeStore.setProcessingForConversation(activeConversationId, false); + } } }