Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 25 additions & 22 deletions packages/core/src/services/gitService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,30 @@ import {
export const SHADOW_REPO_AUTHOR_NAME = 'Gemini CLI';
export const SHADOW_REPO_AUTHOR_EMAIL = 'gemini-cli@google.com';

const SHADOW_REPO_UNSAFE_OPTIONS = {
allowUnsafeAlias: true,
allowUnsafeAskPass: true,
allowUnsafeConfigEnvCount: true,
allowUnsafeConfigPaths: true,
allowUnsafeCredentialHelper: true,
allowUnsafeCustomBinary: true,
allowUnsafeDiffExternal: true,
allowUnsafeDiffTextConv: true,
allowUnsafeEditor: true,
allowUnsafeFilter: true,
allowUnsafeFsMonitor: true,
allowUnsafeGitProxy: true,
allowUnsafeGpgProgram: true,
allowUnsafeHooksPath: true,
allowUnsafeMergeDriver: true,
allowUnsafePack: true,
allowUnsafePager: true,
allowUnsafeProtocolOverride: true,
allowUnsafeSshCommand: true,
allowUnsafeTemplateDir: true,
} satisfies NonNullable<SimpleGitOptions['unsafe']> &
Record<`allowUnsafe${string}`, boolean>;

/**
* Common configuration for the shadow Git repository used for checkpointing.
*
Expand All @@ -32,28 +56,7 @@ export const SHADOW_REPO_AUTHOR_EMAIL = 'gemini-cli@google.com';
* regardless of the user's local environment (e.g., PAGER, EDITOR, or SSH settings).
*/
const SHADOW_REPO_GIT_OPTIONS: Partial<SimpleGitOptions> = {
unsafe: {
allowUnsafeAlias: true,
allowUnsafeAskPass: true,
allowUnsafeConfigEnvCount: true,
allowUnsafeConfigPaths: true,
allowUnsafeCredentialHelper: true,
allowUnsafeCustomBinary: true,
allowUnsafeDiffExternal: true,
allowUnsafeDiffTextConv: true,
allowUnsafeEditor: true,
allowUnsafeFilter: true,
allowUnsafeFsMonitor: true,
allowUnsafeGitProxy: true,
allowUnsafeGpgProgram: true,
allowUnsafeHooksPath: true,
allowUnsafeMergeDriver: true,
allowUnsafePack: true,
allowUnsafePager: true,
allowUnsafeProtocolOverride: true,
allowUnsafeSshCommand: true,
allowUnsafeTemplateDir: true,
},
unsafe: SHADOW_REPO_UNSAFE_OPTIONS,
};

export class GitService {
Expand Down
80 changes: 67 additions & 13 deletions packages/core/src/tools/shell.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ describe('ShellTool', () => {
getGeminiClient: vi.fn().mockReturnValue({}),
getShellToolInactivityTimeout: vi.fn().mockReturnValue(1000),
getEnableInteractiveShell: vi.fn().mockReturnValue(false),
isInteractiveShellEnabled: vi.fn().mockReturnValue(false),
getShellBackgroundCompletionBehavior: vi.fn().mockReturnValue('silent'),
getEnableShellOutputEfficiency: vi.fn().mockReturnValue(true),
getSandboxEnabled: vi.fn().mockReturnValue(false),
Expand Down Expand Up @@ -213,7 +214,7 @@ describe('ShellTool', () => {
callback: (event: ShellOutputEvent) => void,
) => {
mockShellOutputCallback = callback;
const match = cmd.match(/pgrep -g 0 >([^ ]+)/);
const match = cmd.match(/_bgpids_file=([^\r\n]+)/);
if (match) {
extractedTmpFile = match[1].replace(/['"]/g, '');
}
Expand Down Expand Up @@ -308,19 +309,22 @@ describe('ShellTool', () => {
resolveExecutionPromise(fullResult);
};

it('should wrap command on linux and parse pgrep output', async () => {
it('should wrap command on linux and parse background PID output', async () => {
const invocation = shellTool.build({ command: 'my-command &' });
const promise = invocation.execute({ abortSignal: mockAbortSignal });

// Simulate pgrep output file creation by the shell command
// Simulate background PID output file creation by the shell command
fs.writeFileSync(extractedTmpFile, `54321${os.EOL}54322${os.EOL}`);

resolveShellExecution({ pid: 54321 });

const result = await promise;
const wrappedCommand = mockShellExecutionService.mock.calls[0][0];

expect(mockShellExecutionService).toHaveBeenCalledWith(
expect.stringMatching(/pgrep -g 0 >.*gemini-shell-.*[/\\]pgrep\.tmp/),
expect.stringMatching(
/_bgpids_file=.*gemini-shell-.*[/\\]bgpids\.tmp['"]?\n\(\n {2}trap 'jobs -p > "\$_bgpids_file"' EXIT/,
),
tempRootDir,
expect.any(Function),
expect.any(AbortSignal),
Expand All @@ -331,19 +335,67 @@ describe('ShellTool', () => {
sandboxManager: expect.any(Object),
}),
);
expect(wrappedCommand).toMatch(
/^_bgpids_file=.*\n\(\n {2}trap 'jobs -p > "\$_bgpids_file"' EXIT\nmy-command &\n\)\n__code=\$\?\nexit \$__code$/,
);
expect(result.llmContent).toContain('Background PIDs: 54322');
// The file should be deleted by the tool
expect(fs.existsSync(extractedTmpFile)).toBe(false);
});

it('should preserve exit code and capture background PIDs when command uses explicit exit', async () => {
const invocation = shellTool.build({
command: "sh -c 'sleep 60 & exit 1'",
});
const promise = invocation.execute({ abortSignal: mockAbortSignal });

fs.writeFileSync(extractedTmpFile, `67890${os.EOL}`);
expect(fs.readFileSync(extractedTmpFile, 'utf8').trim()).toBe('67890');

resolveShellExecution({ exitCode: 1, output: '' });

const result = await promise;
const wrappedCommand = mockShellExecutionService.mock.calls[0][0];

expect(wrappedCommand).toContain(
'trap \'jobs -p > "$_bgpids_file"\' EXIT',
);
expect(wrappedCommand).toContain('sleep 60 & exit 1');
expect(result.llmContent).toContain('Exit Code: 1');
expect(result.llmContent).toContain('Background PIDs: 67890');
expect(fs.existsSync(extractedTmpFile)).toBe(false);
});

it('should disable PTY execution when interactive shell is unavailable', async () => {
(mockConfig.getEnableInteractiveShell as Mock).mockReturnValue(true);
(mockConfig.isInteractiveShellEnabled as Mock).mockReturnValue(false);

const invocation = shellTool.build({ command: 'python --version' });
const promise = invocation.execute({ abortSignal: mockAbortSignal });
resolveShellExecution();

await promise;

expect(mockShellExecutionService).toHaveBeenCalledWith(
expect.any(String),
tempRootDir,
expect.any(Function),
expect.any(AbortSignal),
false,
expect.objectContaining({
pager: 'cat',
}),
);
});

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({ abortSignal: mockAbortSignal });
resolveShellExecution();
await promise;

expect(mockShellExecutionService).toHaveBeenCalledWith(
expect.stringMatching(/pgrep -g 0 >.*gemini-shell-.*[/\\]pgrep\.tmp/),
expect.stringMatching(/_bgpids_file=.*gemini-shell-.*[/\\]bgpids\.tmp/),
tempRootDir,
expect.any(Function),
expect.any(AbortSignal),
Expand All @@ -359,7 +411,7 @@ describe('ShellTool', () => {
await promise;

expect(mockShellExecutionService).toHaveBeenCalledWith(
expect.stringMatching(/pgrep -g 0 >.*gemini-shell-.*[/\\]pgrep\.tmp/),
expect.stringMatching(/_bgpids_file=.*gemini-shell-.*[/\\]bgpids\.tmp/),
tempRootDir,
expect.any(Function),
expect.any(AbortSignal),
Expand All @@ -379,7 +431,7 @@ describe('ShellTool', () => {
await promise;

expect(mockShellExecutionService).toHaveBeenCalledWith(
expect.stringMatching(/pgrep -g 0 >.*gemini-shell-.*[/\\]pgrep\.tmp/),
expect.stringMatching(/_bgpids_file=.*gemini-shell-.*[/\\]bgpids\.tmp/),
subdir,
expect.any(Function),
expect.any(AbortSignal),
Expand All @@ -402,7 +454,7 @@ describe('ShellTool', () => {
await promise;

expect(mockShellExecutionService).toHaveBeenCalledWith(
expect.stringMatching(/pgrep -g 0 >.*gemini-shell-.*[/\\]pgrep\.tmp/),
expect.stringMatching(/_bgpids_file=.*gemini-shell-.*[/\\]bgpids\.tmp/),
path.join(tempRootDir, 'subdir'),
expect.any(Function),
expect.any(AbortSignal),
Expand Down Expand Up @@ -481,7 +533,7 @@ EOF`;
await promise;

expect(mockShellExecutionService).toHaveBeenCalledWith(
expect.stringMatching(/pgrep -g 0 >.*gemini-shell-.*[/\\]pgrep\.tmp/),
expect.stringMatching(/_bgpids_file=.*gemini-shell-.*[/\\]bgpids\.tmp/),
tempRootDir,
expect.any(Function),
expect.any(AbortSignal),
Expand All @@ -508,7 +560,7 @@ EOF`;

const result = await promise;
expect(result.llmContent).toContain('Error: wrapped command failed');
expect(result.llmContent).not.toContain('pgrep');
expect(result.llmContent).not.toContain('background pid output');
expect(result.display).toEqual(
expect.objectContaining({
name: 'Shell',
Expand Down Expand Up @@ -599,7 +651,7 @@ EOF`;
it('should clean up the temp file on synchronous execution error', async () => {
const error = new Error('sync spawn error');
mockShellExecutionService.mockImplementation((cmd: string) => {
const match = cmd.match(/pgrep -g 0 >([^ ]+)/);
const match = cmd.match(/_bgpids_file=([^\r\n]+)/);
if (match) {
extractedTmpFile = match[1].replace(/['"]/g, ''); // remove any quotes if present
// Create the temp file before throwing to simulate it being left behind
Expand All @@ -616,7 +668,7 @@ EOF`;
expect(fs.existsSync(extractedTmpFile)).toBe(false);
});

it('should not log "missing pgrep output" when process is backgrounded', async () => {
it('should not log "missing background pid output" when process is backgrounded', async () => {
vi.useFakeTimers();
const debugErrorSpy = vi.spyOn(debugLogger, 'error');

Expand All @@ -631,7 +683,9 @@ EOF`;

await promise;

expect(debugErrorSpy).not.toHaveBeenCalledWith('missing pgrep output');
expect(debugErrorSpy).not.toHaveBeenCalledWith(
'missing background pid output',
);
});

describe('Streaming to `updateOutput`', () => {
Expand Down
34 changes: 19 additions & 15 deletions packages/core/src/tools/shell.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,15 +108,16 @@ export class ShellToolInvocation extends BaseToolInvocation<
}

/**
* Wraps a command in a subshell `()` to capture background process IDs (PIDs) using pgrep.
* Uses newlines to prevent breaking heredocs or trailing comments.
* Wraps a command in a subshell to capture background process IDs (PIDs)
* using an EXIT trap. Uses newlines to prevent breaking heredocs or trailing
* comments.
*
* @param command The raw command string to execute.
* @param tempFilePath Path to the temporary file where PIDs will be written.
* @param isWindows Whether the current platform is Windows (if true, the command is returned as-is).
* @returns The wrapped command string.
*/
private wrapCommandForPgrep(
private wrapCommandForBackgroundPIDs(
command: string,
tempFilePath: string,
isWindows: boolean,
Expand All @@ -132,7 +133,7 @@ export class ShellToolInvocation extends BaseToolInvocation<
trimmed += ' ';
}
const escapedTempFilePath = escapeShellArg(tempFilePath, 'bash');
return `(\n${trimmed}\n)\n__code=$?; pgrep -g 0 >${escapedTempFilePath} 2>&1; exit $__code;`;
return `_bgpids_file=${escapedTempFilePath}\n(\n trap 'jobs -p > "$_bgpids_file"' EXIT\n${trimmed}\n)\n__code=$?\nexit $__code`;
}

private getContextualDetails(): string {
Expand Down Expand Up @@ -497,10 +498,10 @@ export class ShellToolInvocation extends BaseToolInvocation<
const onAbort = () => combinedController.abort();
try {
tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gemini-shell-'));
tempFilePath = path.join(tempDir, 'pgrep.tmp');
tempFilePath = path.join(tempDir, 'bgpids.tmp');

// pgrep is not available on Windows, so we can't get background PIDs
const commandToExecute = this.wrapCommandForPgrep(
// Windows shells do not support the POSIX jobs output used here.
const commandToExecute = this.wrapCommandForBackgroundPIDs(
strippedCommand,
tempFilePath,
isWindows,
Expand Down Expand Up @@ -651,7 +652,7 @@ export class ShellToolInvocation extends BaseToolInvocation<
}
},
combinedController.signal,
this.context.config.getEnableInteractiveShell(),
this.context.config.isInteractiveShellEnabled(),
{
...shellExecutionConfig,
sessionId: this.context.config?.getSessionId?.() ?? 'default',
Expand Down Expand Up @@ -736,12 +737,15 @@ export class ShellToolInvocation extends BaseToolInvocation<
}

if (tempFileExists) {
const pgrepContent = await fsPromises.readFile(tempFilePath, 'utf8');
const pgrepLines = pgrepContent
const backgroundPIDContent = await fsPromises.readFile(
tempFilePath,
'utf8',
);
const backgroundPIDLines = backgroundPIDContent
.split('\n')
.map((line) => line.trim())
.filter(Boolean);
for (const line of pgrepLines) {
for (const line of backgroundPIDLines) {
if (!/^\d+$/.test(line)) {
if (
line.includes('sysmond service not found') ||
Expand All @@ -750,7 +754,7 @@ export class ShellToolInvocation extends BaseToolInvocation<
) {
continue;
}
debugLogger.error(`pgrep: ${line}`);
debugLogger.error(`background pid output: ${line}`);
}
const pid = Number(line);
if (pid !== result.pid) {
Expand All @@ -759,7 +763,7 @@ export class ShellToolInvocation extends BaseToolInvocation<
}
} else {
if (!signal.aborted && !result.backgrounded) {
debugLogger.error('missing pgrep output');
debugLogger.error('missing background pid output');
}
}
}
Expand Down Expand Up @@ -1089,7 +1093,7 @@ export class ShellTool extends BaseDeclarativeTool<
// Errors are surfaced when parsing commands.
});
const definition = getShellDefinition(
context.config.getEnableInteractiveShell(),
context.config.isInteractiveShellEnabled(),
context.config.getEnableShellOutputEfficiency(),
context.config.getSandboxEnabled(),
);
Expand Down Expand Up @@ -1139,7 +1143,7 @@ export class ShellTool extends BaseDeclarativeTool<

override getSchema(modelId?: string) {
const definition = getShellDefinition(
this.context.config.getEnableInteractiveShell(),
this.context.config.isInteractiveShellEnabled(),
this.context.config.getEnableShellOutputEfficiency(),
this.context.config.getSandboxEnabled(),
);
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/tools/shell_proactive.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ describe('ShellTool Proactive Expansion', () => {
getModeConfig: vi.fn().mockReturnValue({ readonly: false }),
},
getEnableInteractiveShell: vi.fn().mockReturnValue(false),
isInteractiveShellEnabled: vi.fn().mockReturnValue(false),
getEnableShellOutputEfficiency: vi.fn().mockReturnValue(true),
getShellToolInactivityTimeout: vi.fn().mockReturnValue(1000),
} as unknown as Config;
Expand Down
Loading