Terminal agent: improve send_to_terminal output capture and add waitForOutput#312379
Terminal agent: improve send_to_terminal output capture and add waitForOutput#312379meganrogge merged 2 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Improves the send_to_terminal chat agent tool to better support interactive terminal programs by optionally waiting for terminal output to become idle before returning output.
Changes:
- Added
waitForOutput?: booleaninput parameter to the tool schema and invocation params. - Implemented idle-wait logic based on terminal
onDataactivity and scoping output via a start marker. - Updated/added tests for the new schema parameter and
waitForOutputbehavior.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/sendToTerminalTool.ts | Adds waitForOutput support, marker-scoped output, and idle-wait polling based on terminal data events. |
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/sendToTerminalTool.test.ts | Extends mocks for onData/marker support and adds tests for schema + wait-for-idle path. |
Copilot's findings
Comments suppressed due to low confidence (2)
src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/sendToTerminalTool.ts:397
_waitForIdleOutputcreates aCancellationTokenSourcefromtokenbut never cancels it, and the pollingtimeout(pollIntervalMs)isn't token-aware. This can be simplified by usingtoken.isCancellationRequesteddirectly and/or awaitingtimeout(pollIntervalMs, token)so cancellation is observed immediately (rather than up to the next poll interval).
const cts = new CancellationTokenSource(token);
const dataListener = execution.instance.onData(() => {
lastDataTime = Date.now();
});
try {
while (!cts.token.isCancellationRequested && waited < maxWaitMs) {
await timeout(pollIntervalMs);
waited += pollIntervalMs;
src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/sendToTerminalTool.ts:410
- When
waitForOutputis true,_waitForIdleOutputreturnsgetOutput(execution.instance, startMarker)without alastNLineslimit; ifstartMarkeris undefined this can return up to 60k characters from the entire buffer. To avoid oversized tool responses (and token/latency issues), consider applying a line limit (eg last 20/50 lines) or making the limit explicit/configurable for the waitForOutput path as well.
return getOutput(execution.instance, startMarker ?? undefined);
}
- Files reviewed: 2/2 changed files
- Comments generated: 2
| await timeout(2000); | ||
| recentOutput = getOutput(execution.instance, startMarker ?? undefined, { lastNLines: 20 }); | ||
| } |
There was a problem hiding this comment.
invoke now receives a cancellation token but the default (waitForOutput=false) path uses await timeout(2000) without passing the token, so tool cancellation won't interrupt the 2s delay. Consider using the token-aware timeout(millis, token) (and handling CancellationError if needed) so cancelling a chat/tool invocation returns promptly.
This issue also appears in the following locations of the same file:
- line 389
- line 409
- Merge main to pick up multiline/heredoc command support - Pass cancellation token to timeout() calls so tool cancellation interrupts the 2s delay and idle polling promptly - Wrap waitForOutput test in runWithFakedTimers to avoid real wall-clock delays and flakiness Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| }; | ||
| } | ||
|
|
||
| // Register a marker before sending so we can scope output to just the response |
There was a problem hiding this comment.
@meganrogge This make sense. So previously, was the start marker attached to previous response/ earlier in terminal buffer? Or maybe even no marker and we were just reading the last X number of terminal buffer.
Summary
Improves
send_to_terminalto capture more useful output and adds awaitForOutputmode for interactive programs.Changes
Output capture improvements
timeout()calls now pass the cancellation token so tool cancellation returns promptlyNew
waitForOutputparametertrue, waits for the terminal to become idle (no new output for 2s) before returningModel description update
modelDescriptionto tell the LLM about the 20-line default andwaitForOutputoptionTesting
waitForOutputwaitForOutput=trueidle-wait test usingrunWithFakedTimersregisterMarkerandonDataEval impact
Addresses ~3 terminalbench failures where the agent interacted with interactive programs but got stale/insufficient output (blind-maze-explorer, play-zork, decode-braille).
Fixes #312383