fix: handle race condition for compound commands in new terminals #7435
Conversation
- Add compound command detection to identify commands with &&, ||, ;, |, & operators - Implement delay mechanism for compound commands on newly spawned terminals - Ensure shell integration is fully attached before executing compound commands - Improve error handling in TerminalRegistry for race conditions - Add comprehensive tests for compound command handling Fixes #7430
dosubot
bot
added
size:L
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| * @param command The command to check | ||
| * @returns True if the command contains compound operators | ||
| */ | ||
| public static isCompoundCommand(command: string): boolean { |
There was a problem hiding this comment.
I notice we have duplicate isCompoundCommand implementations here and in TerminalRegistry. Would it make sense to extract this to a shared utility to follow DRY principles? We could place it in a TerminalUtils class or similar.
| console.info( | ||
| `[Terminal ${this.id}] Compound command detected on new terminal, ensuring shell integration is ready`, | ||
| ) | ||
| await new Promise((resolve) => setTimeout(resolve, 100)) |
There was a problem hiding this comment.
The 100ms delay is a magic number. Should we extract this to a named constant like SHELL_INTEGRATION_STABILIZATION_DELAY for better maintainability? This would also make it easier to adjust if we find different timing requirements in the future.
| it("should not detect && in strings or comments", () => { | ||
| // These are still detected as compound commands because we check for the operator presence | ||
| // This is intentional to err on the side of caution | ||
| expect(Terminal.isCompoundCommand('echo "&&"')).toBe(true) |
There was a problem hiding this comment.
The comment mentions this is intentional to "err on the side of caution", but detecting operators inside quoted strings could lead to false positives. Have we considered more sophisticated parsing that respects shell quoting rules? For example, echo "&&" doesn't actually create a compound command.
| * @param command The command to check | ||
| * @returns True if the command contains compound operators | ||
| */ | ||
| private static isCompoundCommand(command: string): boolean { |
There was a problem hiding this comment.
This method is private static while the one in Terminal.ts is public static. Should we align the visibility modifiers for consistency? If we're keeping both implementations, they should probably have the same access level.
| expect(terminal.busy).toBe(true) | ||
|
|
||
| // The shellIntegrationReady flag should be set after the delay | ||
| expect((terminal as any).shellIntegrationReady).toBe(true) |
There was a problem hiding this comment.
Using (terminal as any).shellIntegrationReady with type assertion isn't ideal for testing. Would it be cleaner to expose this through a proper getter method like isShellIntegrationReady() for more maintainable tests?
| return env | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
This JSDoc is helpful, but could we expand it to explain why each operator causes race conditions? For example, mentioning that && spawns sequential processes where the first might complete before shell integration attaches would help future maintainers understand the problem better.
hannesrudolph
added
the
Issue/PR - Triage
|
The issue needs scoping |
Fixes #7430
Problem
As reported by @pwilkin, when compound commands (using operators like
&&,||,;, etc.) are executed in newly spawned terminals, the LLM does not receive the output. This occurs because:cd dir) completes very quicklySolution
This PR implements a targeted fix for the race condition:
1. Compound Command Detection
Terminal.isCompoundCommand()method to detect commands with shell operators (&&,||,;,|,&)2. Shell Integration Synchronization
shellIntegrationReadyflag to track when shell integration is fully attached3. Improved Error Handling
TerminalRegistrynow gracefully handles shell execution end events that arrive before terminal is marked as runningTesting
Impact
Thanks @pwilkin for the detailed debugging information that helped identify the root cause!
Important
Fixes race condition for compound commands in new terminals by adding delay to ensure shell integration readiness.
Terminal.isCompoundCommand()to detect compound commands with operators like&&,||,;,|,&.shellIntegrationReadyflag inTerminalto track shell integration status.TerminalRegistryto handle shell execution end events for compound commands.Terminal.compound.spec.tsto test compound command detection and execution.This description was created by
for 182ecc0. You can customize this summary. It will automatically update as commits are pushed.