fix: handle heredoc/multiline commands in terminal tool execution#307960
fix: handle heredoc/multiline commands in terminal tool execution#307960meganrogge merged 5 commits intomicrosoft:mainfrom
Conversation
b1e84db to
cdbf3cb
Compare
…crosoft#288896) Update sendToTerminalTool and execute strategies to properly handle heredoc statements and multiline commands that were failing during file write operations.
cdbf3cb to
b39e9c4
Compare
|
Rebased onto latest
Branch should show green conflict status now. Ready for review whenever convenient. |
meganrogge
left a comment
There was a problem hiding this comment.
Direction looks good — preserving newlines for heredocs and forcing BPM for multiline is the right fix. A few concerns before merge:
Must-fix
- The foreground terminal path in
sendToTerminalTool.ts(~line 356) still collapses newlines; the multiline branch needs to be applied there too. - Multiline detection differs between files (
\r|\ninsendToTerminalTool.tsvs\n-only in the execute strategies). Please share one helper.
Should-fix
- Forcing bracketed paste mode on all platforms may break shells that don't support it (
cmd.exe, minimal POSIX shells). Please gate on shell capability or confirm downstream handles it. - The three execute-strategy edits are near-duplicates — extract a shared
shouldForceBracketedPaste(commandLine)helper.
Nit
- The added tests cover
normalizeCommandForExecution(unchanged in this PR); a test for the foreground-terminal multiline path would be more valuable. - Briefly comment that bypassing
normalizeCommandForExecutionalso intentionally skips.trim()so future readers don't re-add it.
| await execution.instance.sendText(args.command, true, true); | ||
| } else { | ||
| await execution.instance.sendText(normalizeCommandForExecution(args.command), true); | ||
| } |
There was a problem hiding this comment.
The foreground terminal path in this same file (around line 356) still calls sendText(normalizeCommandForExecution(args.command), true), so heredocs/multiline commands sent to a foreground terminal will still have their newlines collapsed. Please apply the same multiline-detection + bracketed-paste-mode branch there, ideally via a small shared helper so the logic doesn't diverge.
| } | ||
|
|
||
| await execution.instance.sendText(normalizeCommandForExecution(args.command), true); | ||
| const isMultiLine = args.command.includes('\n') || args.command.includes('\r'); |
There was a problem hiding this comment.
Detection here is \n or \r, but the execute strategies only check \n. A command containing only \r would take different branches in the two layers. Please extract a shared helper (e.g. isMultilineCommand(command) using /\r|\n/.test(command)) and use it everywhere so the definition stays consistent.
Also note this path intentionally skips normalizeCommandForExecution's .trim(), which is correct for heredocs but worth a brief comment so future readers don't "fix" it.
| markerRecreation.dispose(); | ||
| const forceBracketedPasteMode = isMacintosh; | ||
| const isMultiLine = commandLine.includes('\n'); | ||
| const forceBracketedPasteMode = isMacintosh || isMultiLine; |
There was a problem hiding this comment.
Two concerns here (same applies to noneExecuteStrategy.ts and richExecuteStrategy.ts):
- Previously BPM was forced only on macOS. Forcing it for any multiline command on all platforms means shells that don't support bracketed paste (e.g.
cmd.exe, minimal POSIX shells) may receive literalESC[200~…ESC[201~markers. Can we gate this on a "shell supports BPM" check, or confirm downstream (sendText/runCommand) already handles the capability? - This same 2-line change is duplicated across three execute strategies. Please extract a
shouldForceBracketedPaste(commandLine)helper so the rule (and any future capability check) lives in one place.
| strictEqual(normalizeCommandForExecution('ls -la'), 'ls -la'); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
These tests exercise normalizeCommandForExecution, which wasn't changed in this PR, so they add little coverage for the fix. More valuable would be a test for the foreground-terminal path in sendToTerminalTool.ts asserting that a multiline command preserves newlines and uses bracketed paste mode (mirroring the persistent-path tests below).
… terminal path Signed-off-by: Maruthan G <maruthang4@gmail.com>
|
Thanks for the thorough review @meganrogge — addressed the must-fixes in 435d3d6. Summary: Must-fix
Should-fix (partial)
Nit
Local checks: |
|
@anthonykim1 can you please test on windows and see before/after this change how it behaves? |
@maruthang Having trouble to get "hang" to repro. Tried to force agent to use send_to_terminal and heredoc but it seems smart enough pre-pr to send EOF on its own.
Do you have steps to test the behavior pre-pr? |
|
Thanks for looking at this @anthonykim1! The reason you're seeing the agent sidestep it is exactly what @brendaningram's issue describes — the agent learned to avoid heredocs because they silently fail, and just uses a different strategy. The "hang" only surfaces when the agent is coaxed into using heredoc through Here's a repro that forces it deterministically on Windows/Linux/macOS: Setup
Prompt that forces the bad path Expected vs observed (pre-PR)
Windows-specific notes
Quicker manual repro (bypasses the agent) If you'd rather not coax the model, you can unit-test the exact pre-PR behavior by calling Let me know if you still can't repro and I'll send a short screencap. |
Backslash-newline sequences (line continuations) are now excluded from multi-line detection so the shell can join them into a single logical line instead of forcing bracketed paste mode unnecessarily.
Resolve conflict in sendToTerminalTool.ts: take main's removal of the foreground terminal path and drop the now-unnecessary foreground tests.
meganrogge
left a comment
There was a problem hiding this comment.
Thank you! Added a bit to this to fix benchmark issues

Fixes #288896
Fixes #312260
When the Copilot agent sends heredoc or multiline commands to the terminal,
normalizeCommandForExecution()collapses all newlines to spaces, destroying the heredoc structure. The shell then receives a single line instead of the multi-line heredoc, causing file write failures.Changes
sendToTerminalTool.ts: Detect multiline commands (containing
\nor\r) and send them with bracketed paste mode enabled, bypassingnormalizeCommandForExecution. This preserves newlines so the shell treats the input as a single paste.Execute strategies (basicExecuteStrategy.ts, noneExecuteStrategy.ts, richExecuteStrategy.ts): Force bracketed paste mode for multiline commands on all platforms (previously only forced on macOS).
Shared
isMultilineCommandhelper (runInTerminalHelpers.ts): Extracted into a reusable function used by bothsendToTerminalTooland all three execute strategies. Uses a negative lookbehind ((?<!\\)\n) so that backslash-newline line continuations are not treated as multiline — the shell naturally joins these into a single logical line, so they can be safely normalized instead of requiring bracketed paste mode.normalizeCommandForExecutionapplied tosend_to_terminalpersistent path: The same normalization (collapsing accidental newlines, trimming whitespace) is applied to the persistent/background terminal path, not just foreground terminals.How to test
echo hello \+ newline +world) and verify it's normalized to a single line rather than using bracketed paste mode