Skip to content

Clarify send_to_terminal input semantics and throttle duplicate background input-needed steering#308858

Closed
Copilot wants to merge 2 commits intomainfrom
copilot/fix-code-comments-review-thread-again
Closed

Clarify send_to_terminal input semantics and throttle duplicate background input-needed steering#308858
Copilot wants to merge 2 commits intomainfrom
copilot/fix-code-comments-review-thread-again

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 9, 2026

This addresses review feedback on PR #308855: the send_to_terminal tool contract did not reflect empty-input (Enter) behavior, and background run_in_terminal input-needed detection could emit repeated steering requests.

  • Tool contract alignment (sendToTerminalTool)

    • Updated modelDescription to describe command as terminal input text (not only shell commands).
    • Updated schema docs to explicitly allow empty/whitespace command to send only Enter for interactive prompts.
  • Background steering dedupe/throttle (runInTerminalTool)

    • Added background onDidDetectInputNeeded steering notifications.
    • Added duplicate suppression for repeated input-needed signals with unchanged output within a throttle window.
    • Introduced a named constant for throttle duration (INPUT_NEEDED_NOTIFICATION_THROTTLE_MS).
  • Focused test coverage

    • Updated sendToTerminalTool tests to assert the new contract wording.
    • Added a runInTerminalTool test validating:
      • duplicate rapid input-needed events are suppressed, and
      • a changed prompt/output triggers a new steering notification.
const isDuplicate =
	currentOutput === lastInputNeededOutput &&
	now - lastInputNeededNotificationTime < INPUT_NEEDED_NOTIFICATION_THROTTLE_MS;
if (isDuplicate) {
	return;
}

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • redirector.gvt1.com
    • Triggering command: /proc/self/exe /proc/self/exe --type=utility --utility-sub-type=network.mojom.NetworkService --lang=en-US --service-sandbox-type=none --no-sandbox --crashpad-handler-pid=6751 --enable-crash-reporter=c7f023f8-1f31-42f4-8903-608ea250cc50,no_channel --user-data-dir=/tmp/vscode-tests-1775761267125 --shared-files=v8_context_snapshot_data:100 --field-trial-handle=3,i,6730299450846452631,2722596066885432261,262144 --enable-features=PdfUseShowSaveFilePicker --disable-features=LocalNetworkAccessChecks,ScreenAIOCREnabled,SpareRendererForSitePerProcess,TraceSiteInstanceGetProcessCreation --variations-seed-version --trace-process-track-uuid=3190708989122997041 (dns block)
    • Triggering command: .build/electron/code-oss .build/electron/code-oss test/unit/electron/index.js --crash-reporter-directory=/home/REDACTED/work/vscode/vscode/.build/crashes --run src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/sendToTerminalTool.test.ts --run src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/electron-browser/runInTerminalTool.test.ts (dns block)
    • Triggering command: /proc/self/exe /proc/self/exe --type=utility --utility-sub-type=network.mojom.NetworkService --lang=en-US --service-sandbox-type=none --no-sandbox --crashpad-handler-pid=8414 --enable-crash-reporter=c7f023f8-1f31-42f4-8903-608ea250cc50,no_channel --user-data-dir=/tmp/vscode-tests-1775761452070 --shared-files=v8_context_snapshot_data:100 --field-trial-handle=3,i,246071511576679397,6052411313879373610,262144 --enable-features=PdfUseShowSaveFilePicker --disable-features=LocalNetworkAccessChecks,ScreenAIOCREnabled,SpareRendererForSitePerProcess,TraceSiteInstanceGetProcessCreation --variations-seed-version --trace-process-track-uuid=3190708989122997041 h s false --sourceMap node (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI requested review from Copilot and removed request for Copilot April 9, 2026 18:57
Agent-Logs-Url: https://github.com/microsoft/vscode/sessions/bb2d5d8a-bc84-4d10-8c20-715b4f2daf51

Co-authored-by: meganrogge <29464607+meganrogge@users.noreply.github.com>
Copilot AI requested review from Copilot and removed request for Copilot April 9, 2026 19:16
Copilot AI changed the title [WIP] Fix code based on review comments Clarify send_to_terminal input semantics and throttle duplicate background input-needed steering Apr 9, 2026
Copilot AI requested a review from meganrogge April 9, 2026 19:17
@meganrogge meganrogge marked this pull request as ready for review April 9, 2026 19:21
Copilot AI review requested due to automatic review settings April 9, 2026 19:21
@meganrogge meganrogge closed this Apr 9, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR aligns the terminal chat agent tools’ documented contracts with actual interactive behavior, and reduces repeated “input needed” steering chatter for background run_in_terminal executions.

Changes:

  • Updated send_to_terminal tool documentation to clarify that command is arbitrary terminal input and may be empty/whitespace to send only Enter.
  • Added background input-needed steering notifications to run_in_terminal, with dedupe/throttling to suppress rapid duplicates when output hasn’t changed.
  • Expanded unit tests to validate the updated contract wording and the new dedupe behavior.
Show a summary per file
File Description
src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/electron-browser/runInTerminalTool.test.ts Adds coverage for deduping background input-needed steering requests and extends chat service stubbing to capture steering calls.
src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/sendToTerminalTool.test.ts Updates assertions to reflect the clarified send_to_terminal contract wording.
src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/sendToTerminalTool.ts Updates tool modelDescription and schema docs to describe empty/whitespace input semantics.
src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/runInTerminalTool.ts Introduces throttling/deduping of background input-needed steering notifications.

Copilot's findings

  • Files reviewed: 4/4 changed files
  • Comments generated: 1

Comment on lines +1762 to +1775
(runInTerminalTool.constructor as unknown as { _activeExecutions: Map<string, { getOutput(): string }> })._activeExecutions.set(termId, {
getOutput: () => output,
});

(runInTerminalTool as unknown as { _registerCompletionNotification: (terminal: ITerminalInstance, termId: string, session: URI, commandName: string, outputMonitor: { onDidDetectInputNeeded: Event<void>; continueMonitoringAsync: () => void; dispose: () => void }) => void })
._registerCompletionNotification(terminalInstance, termId, sessionResource, 'npm init', outputMonitor);

inputNeededEmitter.fire();
inputNeededEmitter.fire();
strictEqual(capturedSteeringRequests.length, 1, 'Expected duplicate rapid input-needed events to be suppressed');

output = 'Confirm (y/N):';
inputNeededEmitter.fire();
strictEqual(capturedSteeringRequests.length, 2, 'Expected a changed prompt to trigger a new notification');
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test mutates the private static RunInTerminalTool._activeExecutions map via .set(...) but never removes the entry. Because the map is static, this can leak state across tests and potentially affect later tests (or accumulate over time) within the same test run. Add cleanup (e.g. delete(termId)/clear() in a try/finally, or register a stub execution that can be removed via RunInTerminalTool.removeExecution).

Suggested change
(runInTerminalTool.constructor as unknown as { _activeExecutions: Map<string, { getOutput(): string }> })._activeExecutions.set(termId, {
getOutput: () => output,
});
(runInTerminalTool as unknown as { _registerCompletionNotification: (terminal: ITerminalInstance, termId: string, session: URI, commandName: string, outputMonitor: { onDidDetectInputNeeded: Event<void>; continueMonitoringAsync: () => void; dispose: () => void }) => void })
._registerCompletionNotification(terminalInstance, termId, sessionResource, 'npm init', outputMonitor);
inputNeededEmitter.fire();
inputNeededEmitter.fire();
strictEqual(capturedSteeringRequests.length, 1, 'Expected duplicate rapid input-needed events to be suppressed');
output = 'Confirm (y/N):';
inputNeededEmitter.fire();
strictEqual(capturedSteeringRequests.length, 2, 'Expected a changed prompt to trigger a new notification');
const activeExecutions = (runInTerminalTool.constructor as unknown as { _activeExecutions: Map<string, { getOutput(): string }> })._activeExecutions;
activeExecutions.set(termId, {
getOutput: () => output,
});
try {
(runInTerminalTool as unknown as { _registerCompletionNotification: (terminal: ITerminalInstance, termId: string, session: URI, commandName: string, outputMonitor: { onDidDetectInputNeeded: Event<void>; continueMonitoringAsync: () => void; dispose: () => void }) => void })
._registerCompletionNotification(terminalInstance, termId, sessionResource, 'npm init', outputMonitor);
inputNeededEmitter.fire();
inputNeededEmitter.fire();
strictEqual(capturedSteeringRequests.length, 1, 'Expected duplicate rapid input-needed events to be suppressed');
output = 'Confirm (y/N):';
inputNeededEmitter.fire();
strictEqual(capturedSteeringRequests.length, 2, 'Expected a changed prompt to trigger a new notification');
} finally {
activeExecutions.delete(termId);
}

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants