fix(terminal): address code review findings

Author: edelauna

src/core/tools/ExecuteCommandTool.ts

@@ -518,10 +518,12 @@ export async function executeCommandInTerminal(
 
 	// Wait for onCompleted callback to finish. onCompleted is async and sets
 	// `completed` and `persistedResult`; we must not read them until it resolves.
-	// Previously this only waited when exitDetails was set (i.e. the normal event
-	// path), but onCompleted also fires on the D-marker and zero-chunk grace-timer
-	// paths where exitDetails is undefined — those paths need the same wait.
-	await onCompletedPromise
+	// Skip when returning a background result: the command is still running and
+	// onCompleted will fire later — awaiting it here would block until real completion,
+	// defeating the purpose of the agent-timeout background transition.
+	if (!runInBackground) {
+		await onCompletedPromise
+	}
 
 	if (message) {
 		const { text, images } = message

src/integrations/terminal/TerminalProcess.ts

@@ -86,7 +86,10 @@ export class TerminalProcess extends BaseTerminalProcess {
 			return
 		}
 
-		// Create a promise that resolves when the stream becomes available
+		// Create a promise that resolves when the stream becomes available.
+		// cancelStreamWait() lets the early-completion race path abort the pending
+		// timeout so it doesn't fire (and reject) after we've already returned.
+		let cancelStreamWait: () => void = () => {}
 		const streamAvailable = new Promise<AsyncIterable<string>>((resolve, reject) => {
 			const timeoutId = setTimeout(() => {
 				// Remove event listener to prevent memory leaks
@@ -106,6 +109,11 @@ export class TerminalProcess extends BaseTerminalProcess {
 				)
 			}, Terminal.getShellIntegrationTimeout())
 
+			cancelStreamWait = () => {
+				clearTimeout(timeoutId)
+				this.removeAllListeners("stream_available")
+			}
+
 			// Clean up timeout if stream becomes available
 			this.once("stream_available", (stream: AsyncIterable<string>) => {
 				clearTimeout(timeoutId)
@@ -127,6 +135,16 @@ export class TerminalProcess extends BaseTerminalProcess {
 			})
 		})
 
+		// Register shell_execution_started listener BEFORE awaiting streamAvailable.
+		// BaseTerminal.setActiveStream emits shell_execution_started and stream_available
+		// on the same synchronous tick (in that order). If we register after the await,
+		// we miss the event and shellExecutionStarted stays false, causing the idle-timeout
+		// guard to incorrectly treat a running-but-silent command as stalled.
+		let shellExecutionStarted = false
+		this.once("shell_execution_started", () => {
+			shellExecutionStarted = true
+		})
+
 		// Execute command.
 		// Determine whether the active shell is PowerShell so we can apply the
 		// PS-specific counter/sleep workarounds.  Prefer the Zoo Code profile
@@ -167,16 +185,39 @@ export class TerminalProcess extends BaseTerminalProcess {
 			// VSCode doesn't buffer retroactively, and zero chunks arrive.
 		} catch (error) {
 			this.terminal.activeShellExecution = undefined
+			this.cleanupScriptFile()
 			throw error
 		}
 
 		this.isHot = true
 
-		// Wait for stream to be available
+		// Wait for stream to be available, but also race against shellExecutionComplete.
+		// If the end event fires before the stream arrives (e.g. a zero-output command
+		// where onDidEndTerminalShellExecution beats onDidStartTerminalShellExecution),
+		// we would otherwise block until the stream timeout fires. Resolving early on
+		// completion produces a clean empty-output result instead.
 		let stream: AsyncIterable<string>
 
 		try {
-			stream = await streamAvailable
+			const COMPLETED_BEFORE_STREAM = Symbol("completed_before_stream")
+			const result = await Promise.race([
+				streamAvailable,
+				shellExecutionComplete.then(() => COMPLETED_BEFORE_STREAM as typeof COMPLETED_BEFORE_STREAM),
+			])
+
+			if (result === COMPLETED_BEFORE_STREAM) {
+				console.info("[Terminal Process] shell execution completed before stream arrived — finishing cleanly")
+				cancelStreamWait()
+				this.terminal.activeShellExecution = undefined
+				this.terminal.busy = false
+				this.isHot = false
+				this.cleanupScriptFile()
+				this.emit("completed", "")
+				this.emit("continue")
+				return
+			}
+
+			stream = result as AsyncIterable<string>
 		} catch (error) {
 			// Stream timeout or other error occurred
 			console.error("[Terminal Process] Stream error:", error.message)
@@ -188,6 +229,7 @@ export class TerminalProcess extends BaseTerminalProcess {
 			)
 
 			this.terminal.busy = false
+			this.cleanupScriptFile()
 
 			// Emit continue event to allow execution to proceed
 			this.emit("continue")
@@ -231,19 +273,6 @@ export class TerminalProcess extends BaseTerminalProcess {
 		let idleTimedOut = false
 		const streamStartedAt = Date.now()
 
-		// Guard against the idle timeout misfiring during shell initialization.
-		// On cold shells (e.g. zsh with a heavy .zshrc), waitForShellIntegration
-		// resolves when ]633;A is emitted, but the shell may still be loading by
-		// the time executeCommand() is called. onDidStartTerminalShellExecution
-		// (→ "shell_execution_started") fires only once the command actually starts
-		// running. If it hasn't arrived yet when the 3s idle timer fires, the shell
-		// is still initializing — self-finalizing would silently drop the command's
-		// output, so we wait.
-		let shellExecutionStarted = false
-		this.once("shell_execution_started", () => {
-			shellExecutionStarted = true
-		})
-
 		// VSCode's execution.read() AsyncIterable can stay open indefinitely even after
 		// the command finishes — it has no built-in cancellation. We need to be able to
 		// break out of the loop when onDidEndTerminalShellExecution fires. We do this by
@@ -291,31 +320,33 @@ export class TerminalProcess extends BaseTerminalProcess {
 			}
 
 			if (raceResult === IDLE_SENTINEL) {
-				if (!shellExecutionStarted) {
-					const elapsedMs = Date.now() - streamStartedAt
-					const shellInitTimeout = Terminal.getShellIntegrationTimeout()
-
-					if (elapsedMs < shellInitTimeout) {
-						// onDidStartTerminalShellExecution hasn't fired yet — the shell is
-						// still initializing. Don't self-finalize; re-arm the idle timer
-						// and keep waiting (the DONE_SENTINEL or a real chunk will break
-						// us out once the command actually starts running).
-						console.info(
-							`[Terminal Process] idle timeout fired but shell execution not started yet — waiting for shell init (${elapsedMs}ms elapsed)`,
-						)
-						continue
-					}
-
-					// Shell integration timeout exceeded and onDidStartTerminalShellExecution
-					// never fired — something went wrong during shell init. Fall through to
-					// self-finalize so we don't wait forever.
+				if (shellExecutionStarted) {
+					// The command is confirmed running (shell_execution_started fired). A
+					// silent command like `sleep 5` can legitimately produce zero output —
+					// elapsed time alone is not proof of completion. Re-arm the idle timer
+					// and keep waiting for a real chunk, the D marker, or the end event.
 					console.info(
-						`[Terminal Process] shell execution never started after ${elapsedMs}ms — self-finalizing`,
+						`[Terminal Process] idle timeout fired but shell execution is running — re-arming (${chunkCount} chunks so far)`,
 					)
+					continue
 				}
 
-				// No data arrived within the idle window and onDidEndTerminalShellExecution
-				// hasn't fired either — VSCode is not going to tell us. Self-finalize.
+				const elapsedMs = Date.now() - streamStartedAt
+				const shellInitTimeout = Terminal.getShellIntegrationTimeout()
+
+				if (elapsedMs < shellInitTimeout) {
+					// onDidStartTerminalShellExecution hasn't fired yet — the shell is
+					// still initializing. Don't self-finalize; re-arm the idle timer
+					// and keep waiting.
+					console.info(
+						`[Terminal Process] idle timeout fired but shell execution not started yet — waiting for shell init (${elapsedMs}ms elapsed)`,
+					)
+					continue
+				}
+
+				// Shell integration timeout exceeded and onDidStartTerminalShellExecution
+				// never fired — something went wrong during shell init. Self-finalize.
+				console.info(`[Terminal Process] shell execution never started after ${elapsedMs}ms — self-finalizing`)
 				idleTimedOut = true
 				console.info(
 					`[Terminal Process] idle timeout (${IDLE_TIMEOUT_MS}ms) after ${chunkCount} chunk(s) — self-finalizing`,
@@ -421,15 +452,7 @@ export class TerminalProcess extends BaseTerminalProcess {
 
 		this.terminal.activeShellExecution = undefined
 
-		// Clean up the temp script file if one was written for this command.
-		if (this.scriptPath) {
-			try {
-				fs.unlinkSync(this.scriptPath)
-			} catch {
-				// Best-effort: if it's already gone, that's fine.
-			}
-			this.scriptPath = undefined
-		}
+		this.cleanupScriptFile()
 
 		this.isHot = false
 
@@ -493,7 +516,21 @@ export class TerminalProcess extends BaseTerminalProcess {
 		// We need a known shell executable to run it — if we can't determine one,
 		// fall back to { ... } wrapping (accepts the VSCode zero-chunk bug as a
 		// lesser evil than invoking a non-existent "sh" on Windows).
-		const shellExe = Terminal.getProfileShell()?.shellPath
+		//
+		// Try the Zoo Code profile first; if unset, fall back to the VS Code default
+		// profile so users who haven't configured a Zoo Code profile override still
+		// get the temp-file path instead of { ... } wrapping.
+		let shellExe = Terminal.getProfileShell()?.shellPath
+		if (!shellExe) {
+			const defaultProfileName = Terminal.getConfiguredDefaultProfileName()
+			if (defaultProfileName) {
+				const profiles = Terminal.getConfiguredProfiles()
+				const profile = profiles?.[defaultProfileName] as { path?: string | string[] } | null | undefined
+				if (profile) {
+					shellExe = Terminal.resolveProfilePath(profile.path)
+				}
+			}
+		}
 		if (!shellExe) {
 			return `{\n${command}\n}`
 		}
@@ -509,6 +546,17 @@ export class TerminalProcess extends BaseTerminalProcess {
 
 	private scriptPath: string | undefined
 
+	private cleanupScriptFile() {
+		if (this.scriptPath) {
+			try {
+				fs.unlinkSync(this.scriptPath)
+			} catch {
+				// Best-effort: if it's already gone, that's fine.
+			}
+			this.scriptPath = undefined
+		}
+	}
+
 	public override continue() {
 		this.emitRemainingBufferIfListening()
 		this.isListening = false

src/integrations/terminal/TerminalRegistry.ts

@@ -80,7 +80,13 @@ export class TerminalRegistry {
 						}
 						const stream = e.execution.read()
 						terminal.setActiveStream(stream)
-						terminal.busy = true // Mark terminal as busy when shell execution starts
+						// Only mark busy when there is a live process to clear it later.
+						// If the end event already fired (early-completion race), process is
+						// undefined and setActiveStream returned early — setting busy here would
+						// leave the terminal stuck busy with nothing to clear it.
+						if (terminal.process) {
+							terminal.busy = true
+						}
 					} else {
 						console.error(
 							"[onDidStartTerminalShellExecution] Shell execution started, but not from a Roo-registered terminal:",

src/integrations/terminal/__tests__/TerminalProcess.spec.ts

@@ -634,6 +634,77 @@ describe("TerminalProcess", () => {
 			const calledWith = mockTerminal.shellIntegration.executeCommand.mock.calls[0][0] as string
 			expect(calledWith).toMatch(/^"\/bin\/bash" ".*roo-cmd-.*\.sh"$/)
 		})
+
+		it("uses VS Code default profile shell for temp-script when no Zoo Code profile override is set", async () => {
+			vi.spyOn(Terminal, "getProfileShell").mockReturnValue(undefined)
+			vi.spyOn(Terminal, "getConfiguredDefaultProfileName").mockReturnValue("bash")
+			vi.spyOn(Terminal, "getConfiguredProfiles").mockReturnValue({
+				bash: { path: "/bin/bash" },
+			})
+
+			const command = "echo a\necho b"
+			const runPromise = terminalProcess.run(command)
+			terminalProcess.emit(
+				"stream_available",
+				(async function* () {
+					yield "\x1b]633;C\x07"
+					yield "a\n"
+					yield "b\n"
+					yield "\x1b]633;D\x07"
+				})(),
+			)
+			setTimeout(() => terminalProcess.emit("shell_execution_complete", { exitCode: 0 }), 0)
+
+			await runPromise
+
+			const calledWith = mockTerminal.shellIntegration.executeCommand.mock.calls[0][0] as string
+			expect(calledWith).toMatch(/^"\/bin\/bash" ".*roo-cmd-.*\.sh"$/)
+		})
+
+		it("completes cleanly when shellExecutionComplete fires before stream_available", async () => {
+			const completedOutputs: string[] = []
+			terminalProcess.on("completed", (output) => completedOutputs.push(output ?? ""))
+
+			// Emit shell_execution_complete on the next tick — after run() has registered its
+			// once("shell_execution_complete") listener but before stream_available fires.
+			// This simulates a zero-output command where the end event beats the stream event.
+			setTimeout(() => terminalProcess.emit("shell_execution_complete", { exitCode: 0 }), 0)
+
+			await terminalProcess.run("echo hello")
+
+			expect(completedOutputs).toEqual([""])
+			expect(terminalProcess.isHot).toBe(false)
+			expect(mockTerminalInfo.busy).toBe(false)
+			expect(mockTerminalInfo.activeShellExecution).toBeUndefined()
+		})
+
+		it("does not leave terminal busy when onDidStartTerminalShellExecution fires after early completion", async () => {
+			// Simulate the production race: end event arrives before the stream.
+			// The registry's onDidEndTerminalShellExecution handler (running=false branch) calls
+			// terminal.shellExecutionComplete(), which clears terminal.process = undefined.
+			// A late onDidStartTerminalShellExecution then arrives: setActiveStream() returns
+			// early (no process), and TerminalRegistry must not set busy = true afterward.
+
+			// Step 1: end fires before run() sets running=true (the !terminal.running registry branch).
+			// Drive this by having the shell_execution_complete event clear terminal.process
+			// the same way shellExecutionComplete() does.
+			setTimeout(() => mockTerminalInfo.shellExecutionComplete({ exitCode: 0, signal: undefined }), 0)
+			await terminalProcess.run("echo hello")
+
+			// terminal.process was cleared by shellExecutionComplete().
+			expect(mockTerminalInfo.process).toBeUndefined()
+			expect(mockTerminalInfo.busy).toBe(false)
+
+			// Step 2: late start event arrives — setActiveStream returns early (no process).
+			// Replicate the TerminalRegistry guard: only set busy when process exists.
+			const lateStream = (async function* () {})()
+			mockTerminalInfo.setActiveStream(lateStream)
+			if (mockTerminalInfo.process) {
+				mockTerminalInfo.busy = true
+			}
+
+			expect(mockTerminalInfo.busy).toBe(false)
+		})
 	})
 
 	describe("continue", () => {