diff --git a/integration-tests/run_shell_command.test.ts b/integration-tests/run_shell_command.test.ts index 02fda5be454..7baa78213c5 100644 --- a/integration-tests/run_shell_command.test.ts +++ b/integration-tests/run_shell_command.test.ts @@ -4,6 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ +import { join } from 'node:path'; import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { TestRig, @@ -14,6 +15,7 @@ import { import { getShellConfiguration } from '../packages/core/src/utils/shell-utils.js'; const { shell } = getShellConfiguration(); +const itBashOnly = shell === 'bash' ? it : it.skip; function getLineCountCommand(): { command: string; tool: string } { switch (shell) { @@ -166,6 +168,59 @@ describe('run_shell_command', () => { }); }); + itBashOnly( + 'should preserve trailing newlines for heredoc shell commands', + async () => { + await rig.setup( + 'should preserve trailing newlines for heredoc shell commands', + { + fakeResponsesPath: join( + import.meta.dirname, + 'shell-trailing-newline.responses', + ), + settings: { tools: { core: ['run_shell_command'] } }, + }, + ); + + const result = await rig.run({ + stdin: 'Run the heredoc command exactly as provided.', + approvalMode: 'yolo', + }); + + const foundToolCall = await rig.waitForToolCall( + 'run_shell_command', + 15000, + (args) => JSON.parse(args).command.includes('TRAILING_NEWLINE_20755'), + ); + + if (!foundToolCall || !result.includes('TRAILING_NEWLINE_20755')) { + printDebugInfo(rig, result, { + 'Found tool call': foundToolCall, + ToolLogs: rig.readToolLogs(), + }); + } + + expect(foundToolCall).toBe(true); + + const toolCall = rig + .readToolLogs() + .find((toolCall) => toolCall.toolRequest.name === 'run_shell_command'); + + expect(toolCall).toBeDefined(); + expect(toolCall!.toolRequest.success).toBe(true); + + const parsedArgs = JSON.parse(toolCall!.toolRequest.args) as { + command: string; + }; + expect(parsedArgs.command.endsWith('\n')).toBe(true); + + expect(result).toContain('TRAILING_NEWLINE_20755'); + expect(result).not.toMatch( + /here-document delimited by end-of-file|syntax error: unexpected end of file/i, + ); + }, + ); + it.skip('should run allowed sub-command in non-interactive mode', async () => { await rig.setup('should run allowed sub-command in non-interactive mode'); diff --git a/integration-tests/shell-trailing-newline.responses b/integration-tests/shell-trailing-newline.responses new file mode 100644 index 00000000000..73ed8a7efcd --- /dev/null +++ b/integration-tests/shell-trailing-newline.responses @@ -0,0 +1,2 @@ +{"method":"generateContentStream","response":[{"candidates":[{"content":{"parts":[{"functionCall":{"name":"run_shell_command","args":{"command":"cat <<'EOF'\nTRAILING_NEWLINE_20755\nEOF\n","description":"Run a heredoc command to verify trailing newline preservation."}}}],"role":"model"},"finishReason":"STOP","index":0}]}]} +{"method":"generateContentStream","response":[{"candidates":[{"content":{"parts":[{"text":"TRAILING_NEWLINE_20755"}],"role":"model"},"finishReason":"STOP","index":0}]}]} diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index 245b7f0eee3..1bcf75833ad 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -44,7 +44,10 @@ vi.mock('node:os', async (importOriginal) => { vi.mock('crypto'); vi.mock('../utils/summarizer.js'); -import { initializeShellParsers } from '../utils/shell-utils.js'; +import { + escapeShellArg, + initializeShellParsers, +} from '../utils/shell-utils.js'; import { ShellTool, OUTPUT_UPDATE_INTERVAL_MS } from './shell.js'; import { debugLogger } from '../index.js'; import { type Config } from '../config/config.js'; @@ -301,7 +304,8 @@ describe('ShellTool', () => { const result = await promise; - const wrappedCommand = `(\n${'my-command &'}\n); __code=$?; pgrep -g 0 >${tmpFile} 2>&1; exit $__code;`; + const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); + const wrappedCommand = `(\nmy-command &\n); __code=$?; pgrep -g 0 >${escapedTmpFile} 2>&1; exit $__code;`; expect(mockShellExecutionService).toHaveBeenCalledWith( wrappedCommand, tempRootDir, @@ -319,6 +323,35 @@ describe('ShellTool', () => { expect(fs.existsSync(tmpFile)).toBe(false); }); + it('should preserve trailing spaces after background commands on linux', async () => { + const invocation = shellTool.build({ command: 'my-command & ' }); + const promise = invocation.execute(mockAbortSignal); + resolveShellExecution({ pid: 54321 }); + + // Simulate pgrep output file creation by the shell command + const tmpFile = path.join(os.tmpdir(), 'shell_pgrep_abcdef.tmp'); + fs.writeFileSync(tmpFile, `54321${os.EOL}54322${os.EOL}`); + + const result = await promise; + + const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); + const wrappedCommand = `(\nmy-command & \n); __code=$?; pgrep -g 0 >${escapedTmpFile} 2>&1; exit $__code;`; + expect(mockShellExecutionService).toHaveBeenCalledWith( + wrappedCommand, + tempRootDir, + expect.any(Function), + expect.any(AbortSignal), + false, + expect.objectContaining({ + pager: 'cat', + sanitizationConfig: {}, + sandboxManager: expect.any(Object), + }), + ); + expect(result.llmContent).toContain('Background PIDs: 54322'); + expect(fs.existsSync(tmpFile)).toBe(false); + }); + it('should add a space when command ends with a backslash to prevent escaping newline', async () => { const invocation = shellTool.build({ command: 'ls\\' }); const promise = invocation.execute(mockAbortSignal); @@ -326,7 +359,8 @@ describe('ShellTool', () => { await promise; const tmpFile = path.join(os.tmpdir(), 'shell_pgrep_abcdef.tmp'); - const wrappedCommand = `(\nls\\ \n); __code=$?; pgrep -g 0 >${tmpFile} 2>&1; exit $__code;`; + const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); + const wrappedCommand = `(\nls\\ \n); __code=$?; pgrep -g 0 >${escapedTmpFile} 2>&1; exit $__code;`; expect(mockShellExecutionService).toHaveBeenCalledWith( wrappedCommand, tempRootDir, @@ -344,7 +378,8 @@ describe('ShellTool', () => { await promise; const tmpFile = path.join(os.tmpdir(), 'shell_pgrep_abcdef.tmp'); - const wrappedCommand = `(\nls # comment\n); __code=$?; pgrep -g 0 >${tmpFile} 2>&1; exit $__code;`; + const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); + const wrappedCommand = `(\nls # comment\n); __code=$?; pgrep -g 0 >${escapedTmpFile} 2>&1; exit $__code;`; expect(mockShellExecutionService).toHaveBeenCalledWith( wrappedCommand, tempRootDir, @@ -354,7 +389,6 @@ describe('ShellTool', () => { expect.any(Object), ); }); - it('should use the provided absolute directory as cwd', async () => { const subdir = path.join(tempRootDir, 'subdir'); const invocation = shellTool.build({ @@ -366,7 +400,8 @@ describe('ShellTool', () => { await promise; const tmpFile = path.join(os.tmpdir(), 'shell_pgrep_abcdef.tmp'); - const wrappedCommand = `(\n${'ls'}\n); __code=$?; pgrep -g 0 >${tmpFile} 2>&1; exit $__code;`; + const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); + const wrappedCommand = `(\nls\n); __code=$?; pgrep -g 0 >${escapedTmpFile} 2>&1; exit $__code;`; expect(mockShellExecutionService).toHaveBeenCalledWith( wrappedCommand, subdir, @@ -391,7 +426,8 @@ describe('ShellTool', () => { await promise; const tmpFile = path.join(os.tmpdir(), 'shell_pgrep_abcdef.tmp'); - const wrappedCommand = `(\n${'ls'}\n); __code=$?; pgrep -g 0 >${tmpFile} 2>&1; exit $__code;`; + const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); + const wrappedCommand = `(\nls\n); __code=$?; pgrep -g 0 >${escapedTmpFile} 2>&1; exit $__code;`; expect(mockShellExecutionService).toHaveBeenCalledWith( wrappedCommand, path.join(tempRootDir, 'subdir'), @@ -406,6 +442,91 @@ describe('ShellTool', () => { ); }); + it('should preserve trailing newlines for heredoc commands on linux', async () => { + const invocation = shellTool.build({ + command: `cat <${escapedTmpFile} 2>&1; exit $__code;`; + expect(mockShellExecutionService).toHaveBeenCalledWith( + wrappedCommand, + tempRootDir, + expect.any(Function), + expect.any(AbortSignal), + false, + expect.objectContaining({ + pager: 'cat', + sanitizationConfig: {}, + sandboxManager: expect.any(Object), + }), + ); + }); + + it('should preserve trailing newlines for comment-only commands on linux', async () => { + const invocation = shellTool.build({ + command: `# comment +`, + }); + const promise = invocation.execute(mockAbortSignal); + resolveShellExecution(); + await promise; + + const tmpFile = path.join(os.tmpdir(), 'shell_pgrep_abcdef.tmp'); + const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); + const wrappedCommand = `( +# comment +); __code=$?; pgrep -g 0 >${escapedTmpFile} 2>&1; exit $__code;`; + expect(mockShellExecutionService).toHaveBeenCalledWith( + wrappedCommand, + tempRootDir, + expect.any(Function), + expect.any(AbortSignal), + false, + expect.objectContaining({ + pager: 'cat', + sanitizationConfig: {}, + sandboxManager: expect.any(Object), + }), + ); + }); + + it('should treat only newline sequences as trailing line terminators on linux', async () => { + const invocation = shellTool.build({ + command: 'printf hello\r', + }); + const promise = invocation.execute(mockAbortSignal); + resolveShellExecution(); + await promise; + + const tmpFile = path.join(os.tmpdir(), 'shell_pgrep_abcdef.tmp'); + const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); + const wrappedCommand = `(\nprintf hello\r\n); __code=$?; pgrep -g 0 >${escapedTmpFile} 2>&1; exit $__code;`; + expect(mockShellExecutionService).toHaveBeenCalledWith( + wrappedCommand, + tempRootDir, + expect.any(Function), + expect.any(AbortSignal), + false, + expect.objectContaining({ + pager: 'cat', + sanitizationConfig: {}, + sandboxManager: expect.any(Object), + }), + ); + }); + it('should handle is_background parameter by calling ShellExecutionService.background', async () => { vi.useFakeTimers(); const invocation = shellTool.build({ diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 7ca475808aa..da333cd7263 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -40,6 +40,7 @@ import { import { formatBytes } from '../utils/formatters.js'; import type { AnsiOutput } from '../utils/terminalSerializer.js'; import { + escapeShellArg, getCommandRoots, initializeShellParsers, stripShellWrapper, @@ -106,14 +107,21 @@ export class ShellToolInvocation extends BaseToolInvocation< if (isWindows) { return command; } - let trimmed = command.trim(); - if (!trimmed) { + if (!command.trim()) { return ''; } - if (trimmed.endsWith('\\')) { - trimmed += ' '; + + let wrappedCommand = command; + const hasTrailingLineTerminator = /\r?\n$/.test(wrappedCommand); + + if (!hasTrailingLineTerminator && /\\[^\S\r\n]*$/.test(wrappedCommand)) { + wrappedCommand += ' '; } - return `(\n${trimmed}\n); __code=$?; pgrep -g 0 >${tempFilePath} 2>&1; exit $__code;`; + + const closingNewline = hasTrailingLineTerminator ? '' : '\n'; + const escapedTempFilePath = escapeShellArg(tempFilePath, 'bash'); + + return `(\n${wrappedCommand}${closingNewline}); __code=$?; pgrep -g 0 >${escapedTempFilePath} 2>&1; exit $__code;`; } private getContextualDetails(): string { @@ -434,7 +442,9 @@ export class ShellToolInvocation extends BaseToolInvocation< options?: ExecuteOptions, ): Promise { const { shellExecutionConfig, setExecutionIdCallback } = options ?? {}; - const strippedCommand = stripShellWrapper(this.params.command); + const strippedCommand = stripShellWrapper(this.params.command, { + preserveTrailingWhitespace: true, + }); if (signal.aborted) { return { diff --git a/packages/core/src/utils/shell-utils.test.ts b/packages/core/src/utils/shell-utils.test.ts index 0dda7c48815..b61b7ebebce 100644 --- a/packages/core/src/utils/shell-utils.test.ts +++ b/packages/core/src/utils/shell-utils.test.ts @@ -392,6 +392,36 @@ describe('stripShellWrapper', () => { it('should not strip anything if no wrapper is present', () => { expect(stripShellWrapper('ls -l')).toEqual('ls -l'); }); + + it('should preserve trailing newlines for wrapped execution commands', () => { + expect( + stripShellWrapper( + `bash -c "cat < { + expect( + stripShellWrapper( + `bash -c "# comment +"`, + { + preserveTrailingWhitespace: true, + }, + ), + ).toEqual(`# comment +`); + }); }); describe('escapeShellArg', () => { diff --git a/packages/core/src/utils/shell-utils.ts b/packages/core/src/utils/shell-utils.ts index 8486be0de9f..6e8cf95e7ae 100644 --- a/packages/core/src/utils/shell-utils.ts +++ b/packages/core/src/utils/shell-utils.ts @@ -796,21 +796,57 @@ export function getCommandRoots(command: string): string[] { .filter(Boolean); } -export function stripShellWrapper(command: string): string { +interface StripShellWrapperOptions { + preserveTrailingWhitespace?: boolean; +} + +function stripSurroundingQuotes( + command: string, + preserveTrailingWhitespace: boolean, +): string { + if (!command) { + return command; + } + + if (!preserveTrailingWhitespace) { + let trimmedCommand = command.trim(); + if ( + (trimmedCommand.startsWith('"') && trimmedCommand.endsWith('"')) || + (trimmedCommand.startsWith("'") && trimmedCommand.endsWith("'")) + ) { + trimmedCommand = trimmedCommand.substring(1, trimmedCommand.length - 1); + } + return trimmedCommand; + } + + const firstChar = command[0]; + if (firstChar !== '"' && firstChar !== "'") { + return command; + } + + const trimmedEnd = command.trimEnd(); + if (!trimmedEnd.endsWith(firstChar)) { + return command; + } + + return trimmedEnd.substring(1, trimmedEnd.length - 1); +} + +export function stripShellWrapper( + command: string, + options: StripShellWrapperOptions = {}, +): string { + const { preserveTrailingWhitespace = false } = options; const pattern = /^\s*(?:(?:(?:\S+\/)?(?:sh|bash|zsh))\s+-c|cmd\.exe\s+\/c|powershell(?:\.exe)?\s+(?:-NoProfile\s+)?-Command|pwsh(?:\.exe)?\s+(?:-NoProfile\s+)?-Command)\s+/i; const match = command.match(pattern); if (match) { - let newCommand = command.substring(match[0].length).trim(); - if ( - (newCommand.startsWith('"') && newCommand.endsWith('"')) || - (newCommand.startsWith("'") && newCommand.endsWith("'")) - ) { - newCommand = newCommand.substring(1, newCommand.length - 1); - } - return newCommand; + return stripSurroundingQuotes( + command.substring(match[0].length), + preserveTrailingWhitespace, + ); } - return command.trim(); + return preserveTrailingWhitespace ? command : command.trim(); } /**