Conversation
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughChangesThe terminal execution path now uses event-driven shell readiness, execution identity checks, race-aware stream handling, multiline temporary scripts, and faster tool-result finalization. New unit and Linux VS Code E2E coverage exercises shell startup, completion, reuse, silent commands, and zero-chunk races. Terminal execution reliability
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant ExecuteCommandTool
participant TerminalProcess
participant VSCodeShell
participant TerminalRegistry
ExecuteCommandTool->>TerminalProcess: run command
TerminalProcess->>VSCodeShell: await shell integration and execute
VSCodeShell->>TerminalRegistry: emit start/end execution events
TerminalRegistry->>TerminalProcess: provide matching stream and completion
TerminalProcess-->>ExecuteCommandTool: return command result
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
edelauna
changed the title
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
edelauna
force-pushed
the
issue/800
branch
2 times, most recently
from
094ae8a to
a84430a
Compare
…hell-integration completion signals
…ution to fix cold-terminal zero-chunk output loss
edelauna
changed the title
edelauna
force-pushed
the
issue/800
branch
2 times, most recently
from
ec1a244 to
596ab97
Compare
edelauna
requested review from
JamesRobert20,
hannesrudolph,
navedmerchant and
taltas
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/integrations/terminal/__tests__/TerminalRegistry.spec.ts (1)
344-387: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding coverage for the
ownExecution === undefinedtransient window.Both new tests here pre-assign
ownExecutionbefore dispatching the event, so they don't exercise the case where a stale start event arrives while the new process'sownExecutionis stillundefined(see the corresponding concern raised onTerminalRegistry.tslines 58-89). Adding a test for that specific ordering would pin down current behavior and guard against regressions once/if the fallback logic is tightened.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/integrations/terminal/__tests__/TerminalRegistry.spec.ts` around lines 344 - 387, The tests should cover a start event arriving while the active TerminalProcess has ownExecution undefined. Add a case alongside the existing startHandler tests that creates the process without assigning ownExecution, dispatches an execution event, and verifies the transient-window behavior: the event must not incorrectly replace the active stream or invoke read unless that is the established expected behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/vscode-e2e/src/suite/tools/fast-exit-shell-race.test.ts`:
- Line 93: Update the timeout in the fast-exit shell race test to 60_000ms,
matching the surrounding test cohort, and remove the temporary diagnostic
comment.
In `@src/core/tools/__tests__/executeCommandTool.spec.ts`:
- Line 195: Update the test assertion in the executeCommandTool test to verify
the custom working directory, such as /custom/path, rather than only checking
for the generic “Command” text. Assert the directory in the returned result or
the terminal creation arguments, while preserving the existing
command-completion coverage.
In `@src/integrations/terminal/TerminalProcess.ts`:
- Around line 549-556: Harden temporary script creation in the command-building
method around scriptPath by using an exclusive file creation mechanism or a
private directory instead of a guessable filename under os.tmpdir(). Preserve
the returned shell command path, and if using fs.mkdtempSync, update
cleanupScriptFile() to remove the created directory as well as its script.
In `@src/integrations/terminal/TerminalRegistry.ts`:
- Around line 58-89: The execution-ownership guard in the
onDidStartTerminalShellExecution handler must not accept undefined ownExecution
for reused terminals. Update the logic around TerminalProcess and
terminal.process so the undefined-ownExecution fallback is allowed only for
genuinely cold, newly created terminals, while reused terminals require an
execution match before calling read() and setActiveStream().
---
Nitpick comments:
In `@src/integrations/terminal/__tests__/TerminalRegistry.spec.ts`:
- Around line 344-387: The tests should cover a start event arriving while the
active TerminalProcess has ownExecution undefined. Add a case alongside the
existing startHandler tests that creates the process without assigning
ownExecution, dispatches an execution event, and verifies the transient-window
behavior: the event must not incorrectly replace the active stream or invoke
read unless that is the established expected behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3d9cb986-5221-4471-b98c-9ba55f032765
📒 Files selected for processing (28)
apps/vscode-e2e/fixtures/cold-shell-init.jsonapps/vscode-e2e/fixtures/fast-exit-shell-race.jsonapps/vscode-e2e/fixtures/long-running-silent-command.jsonapps/vscode-e2e/fixtures/terminal-reuse-shell-race.jsonapps/vscode-e2e/fixtures/zero-chunk-shell-race.jsonapps/vscode-e2e/src/fixtures/cold-shell-init.tsapps/vscode-e2e/src/fixtures/fast-exit-shell-race.tsapps/vscode-e2e/src/fixtures/long-running-silent-command.tsapps/vscode-e2e/src/fixtures/terminal-reuse-shell-race.tsapps/vscode-e2e/src/fixtures/zero-chunk-shell-race.tsapps/vscode-e2e/src/runTest.tsapps/vscode-e2e/src/suite/tools/cold-shell-init.test.tsapps/vscode-e2e/src/suite/tools/fast-exit-shell-race.test.tsapps/vscode-e2e/src/suite/tools/long-running-silent-command.test.tsapps/vscode-e2e/src/suite/tools/terminal-reuse-shell-race.test.tsapps/vscode-e2e/src/suite/tools/zero-chunk-shell-race.test.tssrc/__mocks__/vscode.jssrc/core/tools/ExecuteCommandTool.tssrc/core/tools/__tests__/executeCommandTool.spec.tssrc/integrations/terminal/Terminal.tssrc/integrations/terminal/TerminalProcess.tssrc/integrations/terminal/TerminalRegistry.tssrc/integrations/terminal/__tests__/TerminalProcess.spec.tssrc/integrations/terminal/__tests__/TerminalProcessExec.bash.spec.tssrc/integrations/terminal/__tests__/TerminalProcessExec.cmd.spec.tssrc/integrations/terminal/__tests__/TerminalProcessExec.pwsh.spec.tssrc/integrations/terminal/__tests__/TerminalProfile.spec.tssrc/integrations/terminal/__tests__/TerminalRegistry.spec.ts
github-actions
Bot
added
awaiting-review
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/integrations/terminal/TerminalProcess.ts (1)
293-331: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winConcurrent
iterator.next()calls skip the first output chunk.When the idle timeout fires (line 322) and
shellExecutionStartedis true, thecontinueon line 331 re-enters the loop and callsiterator.next()again on line 294 while the previous.next()promise is still pending. This violates the async iterator protocol — the spec states.next()should not be called before the previous call resolves. The first.next()will eventually resolve with chunk A, but the loop is now awaiting the second.next(), which resolves with chunk B. Chunk A is silently lost. Since the first chunk frequently contains the]633;Cstart marker used bymatchAfterVsceStartMarkers(line 368), losing it can cause the entire output to be mis-parsed.🐛 Proposed fix: hoist `nextChunk` outside the loop
// A sentinel for the no-data idle timeout (see below). const IDLE_SENTINEL = Symbol("idle") // How long to wait for the first chunk before assuming the command finished with // no output (VSCode bug: { ... }-wrapped multiline commands often produce zero // stream data AND delay onDidEndTerminalShellExecution by 60+ seconds). // Only applies before any data arrives (chunkCount === 0). const IDLE_TIMEOUT_MS = 3_000 + // Hoist nextChunk outside the loop so that re-arming after an idle timeout + // reuses the same pending .next() promise instead of calling .next() again + // while the previous call is still unresolved (which violates the async + // iterator protocol and can silently skip the first chunk). + let nextChunk = iterator.next() while (true) { - const nextChunk = iterator.next() const racers: Promise<typeof DONE_SENTINEL | typeof IDLE_SENTINEL | IteratorResult<string>>[] = [ nextChunk, shellExecutionComplete.then(() => DONE_SENTINEL as typeof DONE_SENTINEL), ] @@ 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 } + // Advance to the next chunk only after the current one has been consumed. + nextChunk = iterator.next() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/integrations/terminal/TerminalProcess.ts` around lines 293 - 331, Prevent concurrent async-iterator reads in the stream-consumption loop. In the loop surrounding iterator.next(), create and retain the pending nextChunk promise outside the loop, then race that same promise with the idle and completion signals; only request the subsequent iterator result after the current result has resolved, including when re-arming after IDLE_SENTINEL, so the first output chunk is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/integrations/terminal/TerminalProcess.ts`:
- Around line 293-331: Prevent concurrent async-iterator reads in the
stream-consumption loop. In the loop surrounding iterator.next(), create and
retain the pending nextChunk promise outside the loop, then race that same
promise with the idle and completion signals; only request the subsequent
iterator result after the current result has resolved, including when re-arming
after IDLE_SENTINEL, so the first output chunk is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 92e7377b-2e69-4e6e-a61e-c6cfa64ee56e
📒 Files selected for processing (4)
apps/vscode-e2e/src/suite/tools/fast-exit-shell-race.test.tssrc/core/tools/__tests__/executeCommandTool.spec.tssrc/integrations/terminal/TerminalProcess.tssrc/integrations/terminal/TerminalRegistry.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/vscode-e2e/src/suite/tools/fast-exit-shell-race.test.ts
- src/integrations/terminal/TerminalRegistry.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/integrations/terminal/TerminalProcess.ts (1)
280-487: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWrap stream loop and post-loop in try/finally for resource cleanup.
The stream consumption loop (lines 298-409) and post-loop processing (lines 411-487) are not protected by try/finally. If an error occurs (e.g.,
iterator.next()throws, or an event listener throws duringthis.emit(...)), two resources leak:
- Temp script file/directory —
cleanupScriptFile()at line 465 is never reached, leaving themkdtempSyncdirectory andcmd.shon disk.- Async iterator — the manual
iterator.next()loop doesn't calliterator.return()on exit, unlikefor awaitwhich calls it automatically. The underlying VSCode stream iterator may hold open resources.Additionally, "completed" and "continue" events would not be emitted, potentially hanging the caller.
🛡️ Proposed try/finally wrapper
const iterator = stream[Symbol.asyncIterator]() + try { // A unique sentinel to distinguish "shellExecutionComplete won the race" from a real chunk. const DONE_SENTINEL = Symbol("done") // A sentinel for the no-data idle timeout (see below). @@ this.stopHotTimer() this.emit("completed", this.stripCursorSequences(this.removeVSCodeShellIntegration(this.fullOutput))) this.emit("continue") + } catch (error) { + console.error("[Terminal Process] Error during stream processing:", error) + this.emit("completed", `<terminal process error: ${error instanceof Error ? error.message : String(error)}>`) + this.emit("continue") + throw error + } finally { + void iterator.return?.().catch(() => {}) + this.cleanupScriptFile() + }The explicit
this.cleanupScriptFile()at line 465 can be removed since thefinallyblock now handles it on all paths.cleanupScriptFile()is idempotent so keeping both is also safe.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/integrations/terminal/TerminalProcess.ts` around lines 280 - 487, Wrap the manual stream-consumption and post-loop processing in a try/finally. In the finally block, call iterator.return() when available and always invoke cleanupScriptFile(), ensuring both resources are released when iterator.next() or event emission throws; remove the redundant normal-path cleanup call if appropriate. Preserve the existing completion and continue emissions on successful execution.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/integrations/terminal/TerminalProcess.ts`:
- Around line 280-487: Wrap the manual stream-consumption and post-loop
processing in a try/finally. In the finally block, call iterator.return() when
available and always invoke cleanupScriptFile(), ensuring both resources are
released when iterator.next() or event emission throws; remove the redundant
normal-path cleanup call if appropriate. Preserve the existing completion and
continue emissions on successful execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 83e41e38-07c4-4286-818b-4ebda83c56ac
📒 Files selected for processing (4)
apps/vscode-e2e/src/suite/tools/fast-exit-shell-race.test.tssrc/core/tools/__tests__/executeCommandTool.spec.tssrc/integrations/terminal/TerminalProcess.tssrc/integrations/terminal/TerminalRegistry.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/integrations/terminal/TerminalRegistry.ts
- apps/vscode-e2e/src/suite/tools/fast-exit-shell-race.test.ts
- src/core/tools/tests/executeCommandTool.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/integrations/terminal/TerminalProcess.ts`:
- Around line 316-326: Update the Promise.race handling in TerminalProcess so
the stream loop breaks only when raceResult equals DONE_SENTINEL; do not use
doneSignal.done in that condition, allowing a winning IteratorResult chunk to be
appended even if completion fired concurrently. Add a regression test covering
the final-chunk/completion race.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 71f2d49c-bf5d-4f56-ab05-27322ea3cfc8
📒 Files selected for processing (4)
apps/vscode-e2e/src/suite/tools/fast-exit-shell-race.test.tssrc/core/tools/__tests__/executeCommandTool.spec.tssrc/integrations/terminal/TerminalProcess.tssrc/integrations/terminal/TerminalRegistry.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/vscode-e2e/src/suite/tools/fast-exit-shell-race.test.ts
- src/core/tools/tests/executeCommandTool.spec.ts
- src/integrations/terminal/TerminalRegistry.ts
github-actions
Bot
added
the
awaiting-review
1 participant
Related GitHub Issue
Closes: #800
Description
Two related bugs caused terminal output loss and premature task completion on cold terminals:
Root cause — early
execution.read()callTerminalProcess.run()was callingexecution.read()immediately afterexecuteCommand(), before the shell had started executing the command. VSCode does not buffer stream data retroactively: on a cold terminal (e.g. zsh with a heavy.zshrc) the shell takes 3+ seconds to initialise, so the stream window opened before any output was produced — delivering zero chunks. On warm terminals this wasn't visible because the shell was already running and output arrived within the open window.Fix: remove the early
read()call entirely.read()is now always called insideonDidStartTerminalShellExecutioninTerminalRegistry, which fires exactly when the command starts executing — the earliest point at which output is reliably captured. TheactiveShellExecution === e.executionearly-return guard (which silently prevented re-reading on warm terminals) is replaced by anownExecution-based check that still rejects stale events for superseded executions.Secondary fix — idle timeout misfiring during shell initialisation
The 3s idle sentinel in the stream loop (
chunkCount === 0, no end event) was self-finalising beforeonDidStartTerminalShellExecutionhad fired on cold shells — reporting the command as done before it even started. The sentinel now only self-finalises aftershell_execution_startedhas been received; if it fires before that, the timer is re-armed and the loop continues waiting.Also in this PR
pWaitForpolling withonDidChangeTerminalShellIntegrationevent forwaitForShellIntegration(event-driven, no polling)/tmp/roo-cmd-*.sh) to avoid the VSCode{ ... }-wrapping bug that closes the stream beforeread()is called; quote paths to handle spaces (e.g. Git Bash on Windows:"C:\Program Files\Git\bin\bash.exe" "C:\...\roo-cmd-*.sh")TerminalRegistryforonDidEndTerminalShellExecutionTest Procedure
src/integrations/terminal/__tests__/TerminalProcess.spec.tsandTerminalRegistry.spec.tsPre-Submission Checklist
Documentation Updates
Summary by CodeRabbit