abbyswag/qwen-code
1
0
Fork 0
mirror of https://github.com/QwenLM/qwen-code.git synced 2025-12-20 16:57:46 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: add explicit is_background param for shell tool (#445)

Browse Source 
* fix: add explicit background param for shell tool

* fix: explicit param schema

* docs(shelltool): update `is_background` description
This commit is contained in:
Mingholy
2025-08-27 11:32:48 +08:00
committed by GitHub
parent f61a2df519
commit de279b56f3
3 changed files with 255 additions and 41 deletions
Download Patch File Download Diff File Expand all files Collapse all files

180
packages/core/src/tools/shell.test.ts
View File

@@ -99,24 +99,47 @@ describe('ShellTool', () => {
describe('build', () => {
it('should return an invocation for a valid command', () => {
const invocation = shellTool.build({ command: 'ls -l' });
const invocation = shellTool.build({
command: 'ls -l',
is_background: false,
});
expect(invocation).toBeDefined();
});
it('should throw an error for an empty command', () => {
expect(() => shellTool.build({ command: ' ' })).toThrow(
'Command cannot be empty.',
);
expect(() =>
shellTool.build({ command: ' ', is_background: false }),
).toThrow('Command cannot be empty.');
});
it('should throw an error for a non-existent directory', () => {
vi.mocked(fs.existsSync).mockReturnValue(false);
expect(() =>
shellTool.build({ command: 'ls', directory: 'rel/path' }),
shellTool.build({
command: 'ls',
directory: 'rel/path',
is_background: false,
}),
).toThrow(
"Directory 'rel/path' is not a registered workspace directory.",
);
});
it('should include background indicator in description when is_background is true', () => {
const invocation = shellTool.build({
command: 'npm start',
is_background: true,
});
expect(invocation.getDescription()).toContain('[background]');
});
it('should not include background indicator in description when is_background is false', () => {
const invocation = shellTool.build({
command: 'npm test',
is_background: false,
});
expect(invocation.getDescription()).not.toContain('[background]');
});
});
describe('execute', () => {
@@ -141,7 +164,10 @@ describe('ShellTool', () => {
};
it('should wrap command on linux and parse pgrep output', async () => {
const invocation = shellTool.build({ command: 'my-command &' });
const invocation = shellTool.build({
command: 'my-command &',
is_background: false,
});
const promise = invocation.execute(mockAbortSignal);
resolveShellExecution({ pid: 54321 });
@@ -162,9 +188,81 @@ describe('ShellTool', () => {
expect(vi.mocked(fs.unlinkSync)).toHaveBeenCalledWith(tmpFile);
});
it('should add ampersand to command when is_background is true and command does not end with &', async () => {
const invocation = shellTool.build({
command: 'npm start',
is_background: true,
});
const promise = invocation.execute(mockAbortSignal);
resolveShellExecution({ pid: 54321 });
vi.mocked(fs.existsSync).mockReturnValue(true);
vi.mocked(fs.readFileSync).mockReturnValue('54321\n54322\n');
await promise;
const tmpFile = path.join(os.tmpdir(), 'shell_pgrep_abcdef.tmp');
const wrappedCommand = `{ npm start & }; __code=$?; pgrep -g 0 >${tmpFile} 2>&1; exit $__code;`;
expect(mockShellExecutionService).toHaveBeenCalledWith(
wrappedCommand,
expect.any(String),
expect.any(Function),
mockAbortSignal,
);
});
it('should not add extra ampersand when is_background is true and command already ends with &', async () => {
const invocation = shellTool.build({
command: 'npm start &',
is_background: true,
});
const promise = invocation.execute(mockAbortSignal);
resolveShellExecution({ pid: 54321 });
vi.mocked(fs.existsSync).mockReturnValue(true);
vi.mocked(fs.readFileSync).mockReturnValue('54321\n54322\n');
await promise;
const tmpFile = path.join(os.tmpdir(), 'shell_pgrep_abcdef.tmp');
const wrappedCommand = `{ npm start & }; __code=$?; pgrep -g 0 >${tmpFile} 2>&1; exit $__code;`;
expect(mockShellExecutionService).toHaveBeenCalledWith(
wrappedCommand,
expect.any(String),
expect.any(Function),
mockAbortSignal,
);
});
it('should not add ampersand when is_background is false', async () => {
const invocation = shellTool.build({
command: 'npm test',
is_background: false,
});
const promise = invocation.execute(mockAbortSignal);
resolveShellExecution({ pid: 54321 });
vi.mocked(fs.existsSync).mockReturnValue(true);
vi.mocked(fs.readFileSync).mockReturnValue('54321\n54322\n');
await promise;
const tmpFile = path.join(os.tmpdir(), 'shell_pgrep_abcdef.tmp');
const wrappedCommand = `{ npm test; }; __code=$?; pgrep -g 0 >${tmpFile} 2>&1; exit $__code;`;
expect(mockShellExecutionService).toHaveBeenCalledWith(
wrappedCommand,
expect.any(String),
expect.any(Function),
mockAbortSignal,
);
});
it('should not wrap command on windows', async () => {
vi.mocked(os.platform).mockReturnValue('win32');
const invocation = shellTool.build({ command: 'dir' });
const invocation = shellTool.build({
command: 'dir',
is_background: false,
});
const promise = invocation.execute(mockAbortSignal);
resolveShellExecution({
rawOutput: Buffer.from(''),
@@ -188,7 +286,10 @@ describe('ShellTool', () => {
it('should format error messages correctly', async () => {
const error = new Error('wrapped command failed');
const invocation = shellTool.build({ command: 'user-command' });
const invocation = shellTool.build({
command: 'user-command',
is_background: false,
});
const promise = invocation.execute(mockAbortSignal);
resolveShellExecution({
error,
@@ -209,15 +310,19 @@ describe('ShellTool', () => {
});
it('should throw an error for invalid parameters', () => {
expect(() => shellTool.build({ command: '' })).toThrow(
'Command cannot be empty.',
);
expect(() =>
shellTool.build({ command: '', is_background: false }),
).toThrow('Command cannot be empty.');
});
it('should throw an error for invalid directory', () => {
vi.mocked(fs.existsSync).mockReturnValue(false);
expect(() =>
shellTool.build({ command: 'ls', directory: 'nonexistent' }),
shellTool.build({
command: 'ls',
directory: 'nonexistent',
is_background: false,
}),
).toThrow(
`Directory 'nonexistent' is not a registered workspace directory.`,
);
@@ -231,7 +336,10 @@ describe('ShellTool', () => {
'summarized output',
);
const invocation = shellTool.build({ command: 'ls' });
const invocation = shellTool.build({
command: 'ls',
is_background: false,
});
const promise = invocation.execute(mockAbortSignal);
resolveExecutionPromise({
output: 'long output',
@@ -264,7 +372,10 @@ describe('ShellTool', () => {
});
vi.mocked(fs.existsSync).mockReturnValue(true); // Pretend the file exists
const invocation = shellTool.build({ command: 'a-command' });
const invocation = shellTool.build({
command: 'a-command',
is_background: false,
});
await expect(invocation.execute(mockAbortSignal)).rejects.toThrow(error);
const tmpFile = path.join(os.tmpdir(), 'shell_pgrep_abcdef.tmp');
@@ -282,7 +393,10 @@ describe('ShellTool', () => {
});
it('should throttle text output updates', async () => {
const invocation = shellTool.build({ command: 'stream' });
const invocation = shellTool.build({
command: 'stream',
is_background: false,
});
const promise = invocation.execute(mockAbortSignal, updateOutputMock);
// First chunk, should be throttled.
@@ -322,7 +436,10 @@ describe('ShellTool', () => {
});
it('should immediately show binary detection message and throttle progress', async () => {
const invocation = shellTool.build({ command: 'cat img' });
const invocation = shellTool.build({
command: 'cat img',
is_background: false,
});
const promise = invocation.execute(mockAbortSignal, updateOutputMock);
mockShellOutputCallback({ type: 'binary_detected' });
@@ -370,7 +487,7 @@ describe('ShellTool', () => {
describe('addCoAuthorToGitCommit', () => {
it('should add co-author to git commit with double quotes', async () => {
const command = 'git commit -m "Initial commit"';
const invocation = shellTool.build({ command });
const invocation = shellTool.build({ command, is_background: false });
const promise = invocation.execute(mockAbortSignal);
// Mock the shell execution to return success
@@ -401,7 +518,7 @@ describe('ShellTool', () => {
it('should add co-author to git commit with single quotes', async () => {
const command = "git commit -m 'Fix bug'";
const invocation = shellTool.build({ command });
const invocation = shellTool.build({ command, is_background: false });
const promise = invocation.execute(mockAbortSignal);
resolveExecutionPromise({
@@ -430,7 +547,7 @@ describe('ShellTool', () => {
it('should handle git commit with additional flags', async () => {
const command = 'git commit -a -m "Add feature"';
const invocation = shellTool.build({ command });
const invocation = shellTool.build({ command, is_background: false });
const promise = invocation.execute(mockAbortSignal);
resolveExecutionPromise({
@@ -459,7 +576,7 @@ describe('ShellTool', () => {
it('should not modify non-git commands', async () => {
const command = 'npm install';
const invocation = shellTool.build({ command });
const invocation = shellTool.build({ command, is_background: false });
const promise = invocation.execute(mockAbortSignal);
resolveExecutionPromise({
@@ -487,7 +604,7 @@ describe('ShellTool', () => {
it('should not modify git commands without -m flag', async () => {
const command = 'git commit';
const invocation = shellTool.build({ command });
const invocation = shellTool.build({ command, is_background: false });
const promise = invocation.execute(mockAbortSignal);
resolveExecutionPromise({
@@ -515,7 +632,7 @@ describe('ShellTool', () => {
it('should handle git commit with escaped quotes in message', async () => {
const command = 'git commit -m "Fix \\"quoted\\" text"';
const invocation = shellTool.build({ command });
const invocation = shellTool.build({ command, is_background: false });
const promise = invocation.execute(mockAbortSignal);
resolveExecutionPromise({
@@ -551,7 +668,7 @@ describe('ShellTool', () => {
});
const command = 'git commit -m "Initial commit"';
const invocation = shellTool.build({ command });
const invocation = shellTool.build({ command, is_background: false });
const promise = invocation.execute(mockAbortSignal);
resolveExecutionPromise({
@@ -586,7 +703,7 @@ describe('ShellTool', () => {
});
const command = 'git commit -m "Test commit"';
const invocation = shellTool.build({ command });
const invocation = shellTool.build({ command, is_background: false });
const promise = invocation.execute(mockAbortSignal);
resolveExecutionPromise({
@@ -617,7 +734,7 @@ describe('ShellTool', () => {
describe('shouldConfirmExecute', () => {
it('should request confirmation for a new command and whitelist it on "Always"', async () => {
const params = { command: 'npm install' };
const params = { command: 'npm install', is_background: false };
const invocation = shellTool.build(params);
const confirmation = await invocation.shouldConfirmExecute(
new AbortController().signal,
@@ -632,7 +749,10 @@ describe('ShellTool', () => {
);
// Should now be whitelisted
const secondInvocation = shellTool.build({ command: 'npm test' });
const secondInvocation = shellTool.build({
command: 'npm test',
is_background: false,
});
const secondConfirmation = await secondInvocation.shouldConfirmExecute(
new AbortController().signal,
);
@@ -640,7 +760,9 @@ describe('ShellTool', () => {
});
it('should throw an error if validation fails', () => {
expect(() => shellTool.build({ command: '' })).toThrow();
expect(() =>
shellTool.build({ command: '', is_background: false }),
).toThrow();
});
});
});
@@ -658,6 +780,7 @@ describe('validateToolParams', () => {
const result = shellTool.validateToolParams({
command: 'ls',
directory: 'test',
is_background: false,
});
expect(result).toBeNull();
});
@@ -674,6 +797,7 @@ describe('validateToolParams', () => {
const result = shellTool.validateToolParams({
command: 'ls',
directory: 'test2',
is_background: false,
});
expect(result).toContain('is not a registered workspace directory');
});
@@ -692,6 +816,7 @@ describe('build', () => {
const invocation = shellTool.build({
command: 'ls',
directory: 'test',
is_background: false,
});
expect(invocation).toBeDefined();
});
@@ -709,6 +834,7 @@ describe('build', () => {
shellTool.build({
command: 'ls',
directory: 'test2',
is_background: false,
}),
).toThrow('is not a registered workspace directory');
});

45
packages/core/src/tools/shell.ts
View File

@@ -37,6 +37,7 @@ export const OUTPUT_UPDATE_INTERVAL_MS = 1000;
export interface ShellToolParams {
command: string;
is_background: boolean;
description?: string;
directory?: string;
}
@@ -60,6 +61,10 @@ class ShellToolInvocation extends BaseToolInvocation<
if (this.params.directory) {
description += ` [in ${this.params.directory}]`;
}
// append background indicator
if (this.params.is_background) {
description += ` [background]`;
}
// append optional (description), replacing any line breaks with spaces
if (this.params.description) {
description += ` (${this.params.description.replace(/\n/g, ' ')})`;
@@ -117,12 +122,20 @@ class ShellToolInvocation extends BaseToolInvocation<
// Add co-author to git commit commands
const processedCommand = this.addCoAuthorToGitCommit(strippedCommand);
const shouldRunInBackground = this.params.is_background;
let finalCommand = processedCommand;
// If explicitly marked as background and doesn't already end with &, add it
if (shouldRunInBackground && !finalCommand.trim().endsWith('&')) {
finalCommand = finalCommand.trim() + ' &';
}
// pgrep is not available on Windows, so we can't get background PIDs
const commandToExecute = isWindows
? processedCommand
? finalCommand
: (() => {
// wrap command to append subprocess pids (via pgrep) to temporary file
let command = processedCommand.trim();
let command = finalCommand.trim();
if (!command.endsWith('&')) command += ';';
return `{ ${command} }; __code=$?; pgrep -g 0 >${tempFilePath} 2>&1; exit $__code;`;
})();
@@ -343,7 +356,26 @@ export class ShellTool extends BaseDeclarativeTool<
super(
ShellTool.Name,
'Shell',
`This tool executes a given shell command as \`bash -c <command>\`. Command can start background processes using \`&\`. Command is executed as a subprocess that leads its own process group. Command process group can be terminated as \`kill -- -PGID\` or signaled as \`kill -s SIGNAL -- -PGID\`.
`This tool executes a given shell command as \`bash -c <command>\`.
**Background vs Foreground Execution:**
You should decide whether commands should run in background or foreground based on their nature:
**Use background execution (is_background: true) for:**
- Long-running development servers: \`npm run start\`, \`npm run dev\`, \`yarn dev\`, \`bun run start\`
- Build watchers: \`npm run watch\`, \`webpack --watch\`
- Database servers: \`mongod\`, \`mysql\`, \`redis-server\`
- Web servers: \`python -m http.server\`, \`php -S localhost:8000\`
- Any command expected to run indefinitely until manually stopped
**Use foreground execution (is_background: false) for:**
- One-time commands: \`ls\`, \`cat\`, \`grep\`
- Build commands: \`npm run build\`, \`make\`
- Installation commands: \`npm install\`, \`pip install\`
- Git operations: \`git commit\`, \`git push\`
- Test runs: \`npm test\`, \`pytest\`
Command is executed as a subprocess that leads its own process group. Command process group can be terminated as \`kill -- -PGID\` or signaled as \`kill -s SIGNAL -- -PGID\`.
The following information is returned:
@@ -364,6 +396,11 @@ export class ShellTool extends BaseDeclarativeTool<
type: 'string',
description: 'Exact bash command to execute as `bash -c <command>`',
},
is_background: {
type: 'boolean',
description:
'Whether to run the command in background. Default is false. Set to true for long-running processes like development servers, watchers, or daemons that should continue running without blocking further commands.',
},
description: {
type: 'string',
description:
@@ -375,7 +412,7 @@ export class ShellTool extends BaseDeclarativeTool<
'(OPTIONAL) Directory to run the command in, if not the project root directory. Must be relative to the project root directory and must already exist.',
},
},
required: ['command'],
required: ['command', 'is_background'],
},
false, // output is not markdown
true, // output can be updated