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

Author: edelauna

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/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/runTest.ts

@@ -8,6 +8,7 @@ import { LLMock } from "@copilotkit/aimock"
 
 import { addApplyDiffResultFixtures } from "./fixtures/apply-diff"
 import { addExecuteCommandResultFixtures } from "./fixtures/execute-command"
+import { addFastExitShellRaceResultFixtures } from "./fixtures/fast-exit-shell-race"
 import { addTerminalProfileResultFixtures } from "./fixtures/terminal-profile"
 import { addListFilesResultFixtures } from "./fixtures/list-files"
 import { addReadFileResultFixtures } from "./fixtures/read-file"
@@ -110,6 +111,7 @@ async function main() {
 			if (!isRecord) {
 				addApplyDiffResultFixtures(mock)
 				addExecuteCommandResultFixtures(mock)
+				addFastExitShellRaceResultFixtures(mock)
 				addTerminalProfileResultFixtures(mock)
 				addListFilesResultFixtures(mock)
 				addReadFileResultFixtures(mock)

apps/vscode-e2e/src/suite/tools/fast-exit-shell-race.test.ts

@@ -0,0 +1,116 @@
+/**
+ * Regression test for a real VS Code shell-integration race: a fast-exiting,
+ * multi-line command (wrapped by prepareCommandForShellIntegration into a single
+ * `{ ... }` shell execution) can complete in the terminal while VS Code never
+ * delivers the completion signal -- onDidEndTerminalShellExecution never fires and
+ * the shellIntegration.executeCommand().read() stream never closes on its own, even
+ * though the ]633;D marker text IS written into the stream.
+ *
+ * TerminalProcess.run() recovers by detecting the D marker itself directly in the
+ * accumulated stream data (independent of the stream/event ever formally confirming
+ * completion), then waiting only a brief grace period for the real
+ * onDidEndTerminalShellExecution/exit-code event before proceeding without one.
+ *
+ * This is deliberately narrow: it only self-finalizes on positive proof (the marker
+ * text itself), never on a guessed "gone quiet, must be done" timeout -- a genuinely
+ * long-running, silent command (a cold `tsc --noEmit`, a build, etc.) is
+ * indistinguishable from lost-signal by elapsed time alone, so no such guess is made.
+ * If the marker itself is ever lost too, this still hangs, bounded only by the user's
+ * own commandExecutionTimeout / the model's agentTimeout at the tool layer.
+ *
+ * See: https://github.com/microsoft/vscode/issues/316556
+ *      https://github.com/microsoft/vscode/issues/250764
+ *      https://github.com/microsoft/vscode/issues/254724
+ *
+ * This exercises the real VS Code integrated terminal (terminalShellIntegrationDisabled:
+ * false), not the Execa fallback, since the race lives specifically in VS Code's
+ * shell-integration event/stream plumbing.
+ */
+import * as assert from "assert"
+
+import { RooCodeEventName, type ClineMessage } from "@roo-code/types"
+
+import { waitUntilCompleted } from "../utils"
+import { setDefaultSuiteTimeout } from "../test-utils"
+
+suite("Fast-exit shell integration race", function () {
+	if (process.platform !== "linux") {
+		return
+	}
+
+	setDefaultSuiteTimeout(this)
+
+	setup(async () => {
+		try {
+			await globalThis.api.cancelCurrentTask()
+		} catch {
+			// task may not be running
+		}
+	})
+
+	teardown(async () => {
+		try {
+			await globalThis.api.cancelCurrentTask()
+		} catch {
+			// task may not be running
+		}
+	})
+
+	test("completes a fast-exiting multi-line command via the real VS Code terminal", async function () {
+		const api = globalThis.api
+		const messages: ClineMessage[] = []
+		let errorOccurred: string | null = null
+
+		const messageHandler = ({ message }: { message: ClineMessage }) => {
+			messages.push(message)
+			if (message.type === "say" && message.say === "error") {
+				errorOccurred = message.text || "Unknown error"
+			}
+		}
+		api.on(RooCodeEventName.Message, messageHandler)
+
+		const startedAt = Date.now()
+
+		try {
+			// Bounded well under the un-fixed hang (which stalls for the full 60s test
+			// timeout with zero output). TerminalProcess.run() detects the D marker itself
+			// and only waits a brief grace period (~1s) for the real exit code afterward, so
+			// a healthy run finishes in well under 30s; a regression back to the old hang
+			// will blow this timeout.
+			await waitUntilCompleted({
+				api,
+				start: () =>
+					api.startNewTask({
+						configuration: {
+							mode: "code",
+							autoApprovalEnabled: true,
+							alwaysAllowExecute: true,
+							allowedCommands: ["*"],
+							terminalShellIntegrationDisabled: false,
+						},
+						text: "FAST_EXIT_SHELL_RACE_E2E",
+					}),
+				timeout: 100_000, // TEMP: diagnostic probe, see if the marker ever arrives given patience
+			})
+
+			const elapsedMs = Date.now() - startedAt
+
+			assert.strictEqual(errorOccurred, null, `Error occurred: ${errorOccurred}`)
+
+			// The mock's fixture (see fast-exit-shell-race.ts) only responds with
+			// attempt_completion once its predicate has already verified the tool result
+			// contains the actual 'boom' stderr output AND the specific unknown-exit-status
+			// wording -- so reaching completion_result at all is itself proof that content
+			// survived the lost completion signal.
+			const completionMessage = messages.find(
+				(message) => message.type === "say" && message.say === "completion_result",
+			)
+			assert.ok(
+				completionMessage,
+				`Task should have reached attempt_completion instead of hanging on the command (elapsed: ${elapsedMs}ms)`,
+			)
+		} finally {
+			api.off(RooCodeEventName.Message, messageHandler)
+		}
+	})
+})

src/integrations/terminal/TerminalProcess.ts

@@ -19,6 +19,13 @@ export class TerminalProcess extends BaseTerminalProcess {
 	// Guards against overlapping abort retry loops if abort() is called again
 	// while a previous loop is still re-sending Ctrl+C.
 	private aborting = false
+	// The specific VSCode shell execution this process was started with. Kept on the
+	// process (not just terminal.activeShellExecution, which gets reused/reassigned as
+	// soon as the next command starts) so TerminalRegistry can tell a late
+	// onDidEndTerminalShellExecution event for THIS execution apart from one belonging to
+	// whatever command is currently running on the same reused terminal -- see the
+	// self-finalize grace period in run()'s finalize().
+	public ownExecution?: vscode.TerminalShellExecution
 
 	constructor(terminal: Terminal) {
 		super()
@@ -136,6 +143,7 @@ export class TerminalProcess extends BaseTerminalProcess {
 				this.prepareCommandForShellIntegration(commandToExecute, shellKind),
 			)
 
+			this.ownExecution = execution
 			this.terminal.activeShellExecution = execution
 
 			// VS Code only captures data written after read() is first called, so read
@@ -181,8 +189,37 @@ export class TerminalProcess extends BaseTerminalProcess {
 		 * - OSC 633 ; E ; <commandline> [; <nonce>] ST - Explicitly set command line with optional nonce
 		 */
 
-		// Process stream data
+		// Process stream data.
+		//
+		// VSCode bug: on some platforms/shells (observed with multi-line commands and
+		// certain compound single-line commands), the stream can fail to close on its own
+		// and onDidEndTerminalShellExecution can fail to fire, even though the ]633;D end
+		// marker IS written into the stream -- the command visibly finished, but the event
+		// that would normally tell us so is silently dropped. See:
+		// https://github.com/microsoft/vscode/issues/316556,
+		// https://github.com/microsoft/vscode/issues/250764,
+		// https://github.com/microsoft/vscode/issues/254724.
+		//
+		// We can safely self-finalize on the D marker text alone: it is real, positive
+		// proof the shell finished, independent of whether the stream/event machinery
+		// ever confirms it. What we deliberately do NOT do is guess a "gone quiet, must be
+		// done" timeout: legitimate long-running-but-silent commands (a cold `tsc --noEmit`
+		// on a large project easily runs 20-60s with zero interim output) are
+		// indistinguishable from the broken-signal case by elapsed time alone, so any fixed
+		// threshold either fires falsely on real work or is too long to help. If neither the
+		// marker nor the event ever arrives, this still hangs (the pre-existing behavior) --
+		// bounded only by the user's own commandExecutionTimeout / the model's agentTimeout
+		// at the tool layer, both already user-understood, opt-in settings.
+		let sawEndMarker = false
+		let chunkCount = 0
+		const streamStartedAt = Date.now()
+
 		for await (let data of stream) {
+			chunkCount++
+			console.info(
+				`[Terminal Process] stream chunk #${chunkCount} (+${Date.now() - streamStartedAt}ms, ${data.length} chars)`,
+			)
+
 			const match = this.fullOutput === "" ? this.matchAfterVsceStartMarkers(data) : undefined
 
 			if (match !== undefined) {
@@ -207,13 +244,57 @@ export class TerminalProcess extends BaseTerminalProcess {
 			}
 
 			this.startHotTimer(data)
+
+			if (this.matchBeforeVsceEndMarkers(this.fullOutput) !== undefined) {
+				sawEndMarker = true
+				console.info(
+					`[Terminal Process] D marker observed in stream after ${chunkCount} chunk(s), +${Date.now() - streamStartedAt}ms`,
+				)
+				break
+			}
+		}
+
+		if (!sawEndMarker) {
+			console.info(
+				`[Terminal Process] stream ended without a D marker after ${chunkCount} chunk(s), +${Date.now() - streamStartedAt}ms (stream closed naturally, or run() is about to await shellExecutionComplete indefinitely)`,
+			)
 		}
 
 		// Set streamClosed immediately after stream ends.
 		this.terminal.setActiveStream(undefined)
 
-		// Wait for shell execution to complete.
-		await shellExecutionComplete
+		// Wait for shell execution to complete. Normally this resolves promptly via
+		// onDidEndTerminalShellExecution. If we broke out of the loop early because we saw
+		// the D marker ourselves but that event never arrives (the same VSCode bug), give it
+		// a short grace period to still capture the real exit code, then proceed without one
+		// rather than hang indefinitely. Always clear the grace timer so it doesn't linger in
+		// the event loop after shellExecutionComplete wins the race.
+		if (sawEndMarker) {
+			let graceTimer: NodeJS.Timeout | undefined
+			let graceWon = false
+			const grace = new Promise<void>((resolve) => {
+				graceTimer = setTimeout(() => {
+					graceWon = true
+					resolve()
+				}, 1_000)
+			})
+
+			const waitStartedAt = Date.now()
+			await Promise.race([shellExecutionComplete, grace])
+			clearTimeout(graceTimer)
+			console.info(
+				`[Terminal Process] post-marker wait resolved after ${Date.now() - waitStartedAt}ms via ${
+					graceWon ? "grace timer (no onDidEndTerminalShellExecution)" : "shellExecutionComplete"
+				}`,
+			)
+		} else {
+			const waitStartedAt = Date.now()
+			await shellExecutionComplete
+			console.info(
+				`[Terminal Process] shellExecutionComplete resolved after ${Date.now() - waitStartedAt}ms (no D marker was ever seen in the stream)`,
+			)
+		}
+
 		this.terminal.activeShellExecution = undefined
 
 		this.isHot = false

src/integrations/terminal/TerminalRegistry.ts

@@ -102,6 +102,29 @@ export class TerminalRegistry {
 						terminal.activeShellExecution = undefined
 					}
 
+					// Guard against a late end event for an execution that has already been
+					// superseded on this terminal. This can happen when a process self-finalizes
+					// after TerminalProcess's own D-marker grace period elapses without ever
+					// seeing this event (see TerminalProcess.ts's finalize()): the terminal gets
+					// reused for a new command before VSCode's stale event for the OLD command
+					// finally arrives. Without this check, that stale event would call
+					// shellExecutionComplete() on whatever process/exit-code tracking is
+					// currently attached -- the NEW command's -- corrupting its state instead of
+					// being a harmless no-op for the command it actually belongs to.
+					const isStaleExecution =
+						process instanceof TerminalProcess &&
+						process.ownExecution !== undefined &&
+						process.ownExecution !== e.execution
+
+					if (isStaleExecution) {
+						console.info(
+							"[TerminalRegistry] Ignoring stale onDidEndTerminalShellExecution for a superseded execution",
+							{ terminalId: terminal.id, exitCode: e.exitCode },
+						)
+
+						return
+					}
+
 					if (!terminal.running) {
 						// The end event can arrive before setActiveStream() has set
 						// running=true (race between the global VS Code event and the