Conversation
…hell-integration completion signals
Contributor
|
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✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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