fix(terminal): fix output loss on cold terminals by moving read() into onDidStartTerminalShellExecution (#800) (#834)

* fix(terminal): detect D marker directly to recover from lost VSCode shell-integration completion signals

* fix(terminal): move execution.read() into onDidStartTerminalShellExecution to fix cold-terminal zero-chunk output loss

* fix(terminal): update unit tests for read()-in-TerminalRegistry architecture

* fix(terminal): fix Windows CI test timing and improve coverage

* fix(terminal): address code review findings

* fix(terminal): address CodeRabbit review findings

Author: edelauna

apps/vscode-e2e/fixtures/cold-shell-init.json

@@ -0,0 +1,18 @@
+{
+	"fixtures": [
+		{
+			"match": {
+				"userMessage": "COLD_SHELL_INIT_E2E"
+			},
+			"response": {
+				"toolCalls": [
+					{
+						"name": "execute_command",
+						"arguments": "{\"command\":\"python3 -c \\\"\\nimport sys\\nprint('cold-init-ok', file=sys.stdout)\\nsys.exit(0)\\n\\\"\"}",
+						"id": "call_cold_shell_init_001"
+					}
+				]
+			}
+		}
+	]
+}

apps/vscode-e2e/fixtures/fast-exit-shell-race.json

@@ -0,0 +1,18 @@
+{
+	"fixtures": [
+		{
+			"match": {
+				"userMessage": "FAST_EXIT_SHELL_RACE_E2E"
+			},
+			"response": {
+				"toolCalls": [
+					{
+						"name": "execute_command",
+						"arguments": "{\"command\":\"python3 -c \\\"\\nimport sys\\nprint('boom', file=sys.stderr)\\nsys.exit(1)\\n\\\"\"}",
+						"id": "call_fast_exit_shell_race_001"
+					}
+				]
+			}
+		}
+	]
+}

apps/vscode-e2e/fixtures/long-running-silent-command.json

@@ -0,0 +1,18 @@
+{
+	"fixtures": [
+		{
+			"match": {
+				"userMessage": "LONG_RUNNING_SILENT_COMMAND_E2E"
+			},
+			"response": {
+				"toolCalls": [
+					{
+						"name": "execute_command",
+						"arguments": "{\"command\":\"sleep 5\"}",
+						"id": "call_long_running_silent_001"
+					}
+				]
+			}
+		}
+	]
+}

apps/vscode-e2e/fixtures/terminal-reuse-shell-race.json

@@ -0,0 +1,18 @@
+{
+	"fixtures": [
+		{
+			"match": {
+				"userMessage": "TERMINAL_REUSE_SHELL_RACE_E2E"
+			},
+			"response": {
+				"toolCalls": [
+					{
+						"name": "execute_command",
+						"arguments": "{\"command\":\"python3 -c \\\"\\nimport sys\\nprint('first', file=sys.stderr)\\nsys.exit(0)\\n\\\"\"}",
+						"id": "call_terminal_reuse_001"
+					}
+				]
+			}
+		}
+	]
+}

apps/vscode-e2e/fixtures/zero-chunk-shell-race.json

@@ -0,0 +1,18 @@
+{
+	"fixtures": [
+		{
+			"match": {
+				"userMessage": "ZERO_CHUNK_SHELL_RACE_E2E"
+			},
+			"response": {
+				"toolCalls": [
+					{
+						"name": "execute_command",
+						"arguments": "{\"command\":\"python3 -c \\\"\\nimport sys\\nprint('boom', file=sys.stderr)\\nsys.exit(1)\\n\\\"\"}",
+						"id": "call_zero_chunk_shell_race_001"
+					}
+				]
+			}
+		}
+	]
+}

apps/vscode-e2e/src/fixtures/cold-shell-init.ts

@@ -0,0 +1,56 @@
+import type { ChatCompletionRequest, ChatMessage } from "@copilotkit/aimock"
+
+import { LLMock } from "@copilotkit/aimock"
+
+function anyToolResultContains(req: ChatCompletionRequest, ...terms: string[]): boolean {
+	const messages: ChatMessage[] = Array.isArray(req?.messages) ? req.messages : []
+	return messages.some(
+		(msg) =>
+			msg?.role === "tool" &&
+			typeof msg.content === "string" &&
+			terms.every((t) => (msg.content as string).includes(t)),
+	)
+}
+
+export function addColdShellInitFixtures(mock: InstanceType<typeof LLMock>) {
+	// On cold zsh terminals the first execution may produce 0 chunks (VSCode
+	// execution.read() limitation on basic shell integration — see issue #242897).
+	// When the first result is empty, the mock retries the same command so the
+	// second attempt (on the now-warm terminal) captures real output.
+	mock.addFixture({
+		match: {
+			toolCallId: "call_cold_shell_init_001",
+			predicate: (req: ChatCompletionRequest) =>
+				// First attempt returned empty — retry the command
+				!anyToolResultContains(req, "cold-init-ok"),
+		},
+		response: {
+			toolCalls: [
+				{
+					name: "execute_command",
+					arguments: JSON.stringify({
+						command: "python3 -c \"\nimport sys\nprint('cold-init-ok', file=sys.stdout)\nsys.exit(0)\n\"",
+					}),
+					id: "call_cold_shell_init_003",
+				},
+			],
+		},
+	})
+
+	// Match whichever attempt (first or retry) delivers real output — prove
+	// the guard kept the process alive long enough for the output to arrive.
+	mock.addFixture({
+		match: {
+			predicate: (req: ChatCompletionRequest) => anyToolResultContains(req, "cold-init-ok", "Exit code: 0"),
+		},
+		response: {
+			toolCalls: [
+				{
+					name: "attempt_completion",
+					arguments: JSON.stringify({ result: "Cold shell init command completed with real output." }),
+					id: "call_cold_shell_init_002",
+				},
+			],
+		},
+	})
+}

apps/vscode-e2e/src/fixtures/fast-exit-shell-race.ts

@@ -0,0 +1,32 @@
+import { LLMock } from "@copilotkit/aimock"
+
+import { toolResultContains } from "./tool-result"
+
+export function addFastExitShellRaceResultFixtures(mock: InstanceType<typeof LLMock>) {
+	mock.addFixture({
+		match: {
+			toolCallId: "call_fast_exit_shell_race_001",
+			// VSCode drops onDidEndTerminalShellExecution for this command (the race under
+			// test), so TerminalProcess.run() only has the D marker itself as proof of
+			// completion, never a real exit code (see ExecuteCommandTool.ts's
+			// `exitDetails === undefined` branch). Match on the actual stderr output and the
+			// specific unknown-exit-status text so this fixture -- and the e2e assertions it
+			// drives -- would fail if either the output capture or that fallback wording
+			// regressed, instead of passing on any generic "command executed" result.
+			predicate: (req) =>
+				toolResultContains(req, "call_fast_exit_shell_race_001", [
+					"boom",
+					"<VSCE exitDetails == undefined: terminal output and command execution status is unknown.>",
+				]),
+		},
+		response: {
+			toolCalls: [
+				{
+					name: "attempt_completion",
+					arguments: JSON.stringify({ result: "The script ran and printed 'boom' to stderr." }),
+					id: "call_fast_exit_shell_race_002",
+				},
+			],
+		},
+	})
+}

apps/vscode-e2e/src/fixtures/long-running-silent-command.ts

@@ -0,0 +1,32 @@
+import { LLMock } from "@copilotkit/aimock"
+
+import { toolResultContains } from "./tool-result"
+
+export function addLongRuningSilentCommandFixtures(mock: InstanceType<typeof LLMock>) {
+	// `sleep 5` produces no output and completes normally via onDidEndTerminalShellExecution.
+	// The idle timeout must NOT fire here — it must only fire on zero-chunk commands where
+	// the stream stays open AND the event is delayed (the { ... }-wrapped multiline bug).
+	// For `sleep 5`, the stream stays open (so the idle timer is never active after the
+	// first chunk — but actually sleep 5 may produce no chunks either). The distinction
+	// is that `sleep 5` DOES receive onDidEndTerminalShellExecution promptly after exit,
+	// which breaks the loop via DONE_SENTINEL before the 3s idle timer fires.
+	mock.addFixture({
+		match: {
+			toolCallId: "call_long_running_silent_001",
+			predicate: (req) =>
+				toolResultContains(req, "call_long_running_silent_001", [
+					// sleep exits with code 0 — the normal exit status path
+					"Exit code: 0",
+				]),
+		},
+		response: {
+			toolCalls: [
+				{
+					name: "attempt_completion",
+					arguments: JSON.stringify({ result: "The sleep command completed successfully." }),
+					id: "call_long_running_silent_002",
+				},
+			],
+		},
+	})
+}

apps/vscode-e2e/src/fixtures/terminal-reuse-shell-race.ts

@@ -0,0 +1,42 @@
+import { LLMock } from "@copilotkit/aimock"
+
+import { toolResultContains } from "./tool-result"
+
+export function addTerminalReuseShellRaceFixtures(mock: InstanceType<typeof LLMock>) {
+	// First command completes — model issues a second command on the same terminal.
+	// With the temp-script fix, both commands now deliver real output.
+	mock.addFixture({
+		match: {
+			toolCallId: "call_terminal_reuse_001",
+			predicate: (req) => toolResultContains(req, "call_terminal_reuse_001", ["first", "Exit code: 0"]),
+		},
+		response: {
+			toolCalls: [
+				{
+					name: "execute_command",
+					arguments: JSON.stringify({
+						command: "python3 -c \"\nimport sys\nprint('second', file=sys.stderr)\nsys.exit(0)\n\"",
+					}),
+					id: "call_terminal_reuse_002",
+				},
+			],
+		},
+	})
+
+	// Second command on the reused terminal also completes.
+	mock.addFixture({
+		match: {
+			toolCallId: "call_terminal_reuse_002",
+			predicate: (req) => toolResultContains(req, "call_terminal_reuse_002", ["second", "Exit code: 0"]),
+		},
+		response: {
+			toolCalls: [
+				{
+					name: "attempt_completion",
+					arguments: JSON.stringify({ result: "Both commands ran on the reused terminal." }),
+					id: "call_terminal_reuse_003",
+				},
+			],
+		},
+	})
+}

apps/vscode-e2e/src/fixtures/zero-chunk-shell-race.ts

@@ -0,0 +1,25 @@
+import { LLMock } from "@copilotkit/aimock"
+
+import { toolResultContains } from "./tool-result"
+
+export function addZeroChunkShellRaceResultFixtures(mock: InstanceType<typeof LLMock>) {
+	mock.addFixture({
+		match: {
+			toolCallId: "call_zero_chunk_shell_race_001",
+			// The multiline command is now written to a temp script file and executed
+			// via `sh /tmp/roo-cmd-*.sh` to avoid the VSCode { ... }-wrapping bug that
+			// caused the stream to be closed before read() arrived (zero chunks).
+			// The real output ('boom' on stderr) and exit code (1) now reach the model.
+			predicate: (req) => toolResultContains(req, "call_zero_chunk_shell_race_001", ["boom", "Exit code: 1"]),
+		},
+		response: {
+			toolCalls: [
+				{
+					name: "attempt_completion",
+					arguments: JSON.stringify({ result: "The script ran and printed 'boom' to stderr." }),
+					id: "call_zero_chunk_shell_race_002",
+				},
+			],
+		},
+	})
+}