Conversation
…hell-integration completion signals
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…ng to resolve E2E race condition
Related GitHub Issue
Closes: #800
Description
VSCode's shell-integration API can silently fail to signal command completion for certain command shapes:
onDidEndTerminalShellExecutionnever fires and theexecution.read()stream never closes on its own, even though the command has genuinely finished (or, in the worst case, VSCode delivers zero stream data at all, not even the start marker). Without a fallback, the task stalls forever on a "Running" command with no way to recover short of reloading the window — this is the root cause of #800 and several related Discord reports.The fix:
TerminalProcess.run()now detects the]633;Dcompletion marker directly in the accumulated stream data as it arrives, and self-finalizes as soon as it sees it — independent of whether the stream or the event ever confirms completion. It then waits a brief (1s) grace period for the realonDidEndTerminalShellExecutionevent to still capture an accurate exit code, and proceeds without one if it doesn't arrive.What this deliberately does NOT do: guess a "gone quiet, must be done" idle-timeout. An earlier version of this fix included a 15s idle-timeout as a fallback for the case where even the D marker never arrives. That was reverted after live testing showed it fires falsely on legitimate long-running, quiet commands — e.g. a cold
cd src && npx tsc --noEmiton this repo takes ~24s with zero interim output, which is indistinguishable from the broken-signal case by elapsed time alone. Any fixed threshold either fires falsely on real work or is too long to be useful, so this fix only self-finalizes on positive proof (the marker itself).Related fix: while investigating, found that a late/stale
onDidEndTerminalShellExecutionevent for an already-superseded execution could be misapplied to whatever command is currently running on a reused terminal — corrupting its exit code and prematurely clearing its process state.TerminalRegistrynow tracks the specific execution eachTerminalProcesswas started with (ownExecution) and ignores end-events that don't match the terminal's current process, regardless of whatterminal.activeShellExecutioncurrently points to.Observability: added permanent diagnostic logging (
[Terminal Process] stream chunk #N, D-marker detection, and post-marker wait outcome) so this class of bug is diagnosable going forward without needing to re-instrument.Known residual gap
Some command shapes reliably cause VSCode to emit zero stream data at all — not just a lost completion signal, but no data whatsoever, confirmed via a controlled e2e repro (100% reproducible in this environment) for:
python3 -c "..."command containing a literal embedded newline (auto-wrapped byprepareCommandForShellIntegrationinto{ ...\n... })( ... )subshell wrapper (a construct we don't generate ourselves, but a model could)For these, VSCode's own
onDidStartTerminalShellExecutionevent was observed reporting a corruptedcommandfield — the script's stderr output was concatenated directly into what VSCode thinks is the command-line string — direct evidence this is a VSCode-internal shell-integration parser bug, not something addressable by changing our own command-wrapping strategy. Manual live testing after this fix confirmed: normal long-running quiet commands (tsc --noEmit) now work correctly, but this specific narrow class of command shape still hangs, requiring the user to reload the window as before. This is a real, upstream VSCode bug that we could not safely paper over without reintroducing false positives on legitimate commands.Test Procedure
src/integrations/terminal/__tests__/TerminalProcess.spec.tsandTerminalRegistry.spec.ts— new regression tests cover: (1) self-finalizing on the D marker when the event never fires, (2) NOT falsely completing a long-running silent command, (3) a late/stale end-event for a superseded execution not corrupting the next command's state (verified this test fails without theTerminalRegistryfix, confirming it's a real regression guard).apps/vscode-e2e/src/suite/tools/fast-exit-shell-race.test.ts— drives the real VSCode integrated terminal (not the Execa fallback) with a fast-exiting multi-line command, verified against a live, non-mocked VSCode instance.tsc --noEmit-style commands no longer falsely report "still running," while documenting the residual gap above via direct reproduction.Pre-Submission Checklist
Documentation Updates