Skip to content
Closed
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
127 changes: 127 additions & 0 deletions packages/core/src/services/shellExecutionService.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2192,4 +2192,131 @@ describe('ShellExecutionService environment variables', () => {

vi.unstubAllEnvs();
});

it('should inject Full Access (YOLO) env vars on top of the !isInteractive block', async () => {
// Real production flow: shell.ts forces shouldUseNodePty=false when
// approvalMode === YOLO, so execute() runs the child_process path with
// isInteractive=false. Both env blocks compose: `!isInteractive`
// contributes GIT_TERMINAL_PROMPT / GH_PROMPT_DISABLED / GCM_INTERACTIVE
// (and GIT_CONFIG_* shaping); the YOLO block contributes the
// package-manager / installer vars on top.
vi.resetModules();
vi.stubEnv('CI', undefined);
vi.stubEnv('npm_config_yes', undefined);
vi.stubEnv('DEBIAN_FRONTEND', undefined);
vi.stubEnv('GIT_CONFIG_COUNT', undefined);

const { ShellExecutionService } = await import(
'./shellExecutionService.js'
);
const { ApprovalMode } = await import('../policy/types.js');

mockGetPty.mockResolvedValue(null); // child_process fallback
await ShellExecutionService.execute(
'test-cp-yolo-env',
'/',
vi.fn(),
new AbortController().signal,
false, // YOLO real flow: PTY skipped → isInteractive=false
{ ...shellExecutionConfig, approvalMode: ApprovalMode.YOLO },
);

expect(mockCpSpawn).toHaveBeenCalled();
const cpEnv = mockCpSpawn.mock.calls[0][2].env;
// From the YOLO block (this PR):
expect(cpEnv).toHaveProperty('CI', '1');
expect(cpEnv).toHaveProperty('npm_config_yes', 'true');
expect(cpEnv).toHaveProperty('npm_config_fund', 'false');
expect(cpEnv).toHaveProperty('npm_config_audit', 'false');
expect(cpEnv).toHaveProperty('YARN_ENABLE_INTERACTIVE', 'false');
expect(cpEnv).toHaveProperty('DEBIAN_FRONTEND', 'noninteractive');
expect(cpEnv).toHaveProperty('NEEDRESTART_MODE', 'a');
expect(cpEnv).toHaveProperty('PIP_DISABLE_PIP_VERSION_CHECK', '1');
// From the pre-existing !isInteractive block — included here so a
// regression that decouples the two blocks gets caught:
expect(cpEnv).toHaveProperty('GIT_TERMINAL_PROMPT', '0');
expect(cpEnv).toHaveProperty('GH_PROMPT_DISABLED', '1');
expect(cpEnv).toHaveProperty('GCM_INTERACTIVE', 'never');

mockChildProcess.emit('exit', 0, null);
mockChildProcess.emit('close', 0, null);
await new Promise(process.nextTick);
vi.unstubAllEnvs();
});

it('should NOT inject Full Access env vars when approvalMode is DEFAULT', async () => {
vi.resetModules();
vi.stubEnv('CI', undefined);
vi.stubEnv('npm_config_yes', undefined);
vi.stubEnv('DEBIAN_FRONTEND', undefined);

const { ShellExecutionService } = await import(
'./shellExecutionService.js'
);
const { ApprovalMode } = await import('../policy/types.js');

mockGetPty.mockResolvedValue(null);
await ShellExecutionService.execute(
'test-cp-default-env',
'/',
vi.fn(),
new AbortController().signal,
true, // interactive
{ ...shellExecutionConfig, approvalMode: ApprovalMode.DEFAULT },
);

expect(mockCpSpawn).toHaveBeenCalled();
const cpEnv = mockCpSpawn.mock.calls[0][2].env;
expect(cpEnv).not.toHaveProperty('CI');
expect(cpEnv).not.toHaveProperty('npm_config_yes');
expect(cpEnv).not.toHaveProperty('DEBIAN_FRONTEND');
expect(cpEnv).not.toHaveProperty('NEEDRESTART_MODE');

mockChildProcess.emit('exit', 0, null);
mockChildProcess.emit('close', 0, null);
await new Promise(process.nextTick);
vi.unstubAllEnvs();
});

it('should preserve pre-existing user env values in Full Access mode (?? coalesce)', async () => {
vi.resetModules();
vi.stubEnv('CI', '0');
vi.stubEnv('DEBIAN_FRONTEND', 'dialog');

const { ShellExecutionService } = await import(
'./shellExecutionService.js'
);
const { ApprovalMode } = await import('../policy/types.js');

mockGetPty.mockResolvedValue(null);
await ShellExecutionService.execute(
'test-cp-yolo-preserve',
'/',
vi.fn(),
new AbortController().signal,
true,
{
...shellExecutionConfig,
approvalMode: ApprovalMode.YOLO,
sanitizationConfig: {
...shellExecutionConfig.sanitizationConfig,
allowedEnvironmentVariables: ['CI', 'DEBIAN_FRONTEND'],
},
},
);

expect(mockCpSpawn).toHaveBeenCalled();
const cpEnv = mockCpSpawn.mock.calls[0][2].env;
// User-set values take precedence over the YOLO defaults
expect(cpEnv).toHaveProperty('CI', '0');
expect(cpEnv).toHaveProperty('DEBIAN_FRONTEND', 'dialog');
// The non-conflicting Full Access defaults still get injected
expect(cpEnv).toHaveProperty('npm_config_yes', 'true');
expect(cpEnv).toHaveProperty('NEEDRESTART_MODE', 'a');

mockChildProcess.emit('exit', 0, null);
mockChildProcess.emit('close', 0, null);
await new Promise(process.nextTick);
vi.unstubAllEnvs();
});
});
35 changes: 35 additions & 0 deletions packages/core/src/services/shellExecutionService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import {
type SandboxPermissions,
} from './sandboxManager.js';
import type { SandboxConfig } from '../config/config.js';
import { ApprovalMode } from '../policy/types.js';
import { killProcessGroup } from '../utils/process-utils.js';
import {
ExecutionLifecycleService,
Expand Down Expand Up @@ -105,6 +106,10 @@ export interface ShellExecutionConfig {
backgroundCompletionBehavior?: 'inject' | 'notify' | 'silent';
originalCommand?: string;
sessionId?: string;
// When set to YOLO, prepareExecution injects non-interactive env vars
// (CI=1, npm_config_yes=true, DEBIAN_FRONTEND=noninteractive, etc.) so
// package managers and installers auto-confirm instead of hanging on stdin.
approvalMode?: ApprovalMode;
}

/**
Expand Down Expand Up @@ -481,6 +486,36 @@ export class ShellExecutionService {
});
}

// Full Access mode opts the user into "auto-everything". Make common
// installers / package managers run non-interactively so they don't hang
// on prompts like `npx`'s "Ok to proceed? [y]" or `apt`'s "Do you want
// to continue?". Pre-existing user values pass through (?? coalesce).
//
// GIT_TERMINAL_PROMPT, GH_PROMPT_DISABLED, GCM_INTERACTIVE are NOT set
// here: the `!isInteractive` block above already does (in YOLO we force
// shouldUseNodePty=false in shell.ts → isInteractive=false → that block
// runs and overwrites them unconditionally). Listing them here would be
// redundant and the `??` would be misleading — user values for those
// three are not preserved by either path.
//
// Codex CLI does the always-on equivalent at exec (codex-rs
// `UNIFIED_EXEC_ENV`); Claude Code propagates these vars from the user's
// env. We gate on YOLO because outside Full Access the user has not
// opted in to skipping prompts.
if (shellExecutionConfig.approvalMode === ApprovalMode.YOLO) {
Object.assign(baseEnv, {
CI: baseEnv['CI'] ?? '1',
npm_config_yes: baseEnv['npm_config_yes'] ?? 'true',
npm_config_fund: baseEnv['npm_config_fund'] ?? 'false',
npm_config_audit: baseEnv['npm_config_audit'] ?? 'false',
YARN_ENABLE_INTERACTIVE: baseEnv['YARN_ENABLE_INTERACTIVE'] ?? 'false',
DEBIAN_FRONTEND: baseEnv['DEBIAN_FRONTEND'] ?? 'noninteractive',
NEEDRESTART_MODE: baseEnv['NEEDRESTART_MODE'] ?? 'a',
PIP_DISABLE_PIP_VERSION_CHECK:
baseEnv['PIP_DISABLE_PIP_VERSION_CHECK'] ?? '1',
Comment on lines +507 to +515

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

In the real application flow (defined in shell.ts), Full Access (YOLO) mode skips the PTY, which means shouldUseNodePty is false. This causes ShellExecutionService.execute to call childProcessFallback with isInteractive = false, which in turn triggers the if (!isInteractive) block at line 458. That block unconditionally overwrites GIT_TERMINAL_PROMPT, GH_PROMPT_DISABLED, and GCM_INTERACTIVE (lines 475-480), making the nullish coalescing in the YOLO block redundant and failing to respect user-provided values for these variables. To ensure user values are preserved as intended, the YOLO block should use sanitizedEnv as the source. Additionally, per repository rules, avoid using nullish coalescing for defaults in the code; these should be defined in the configuration schema to maintain a single source of truth.

Suggested change
CI: baseEnv['CI'] ?? '1',
npm_config_yes: baseEnv['npm_config_yes'] ?? 'true',
npm_config_fund: baseEnv['npm_config_fund'] ?? 'false',
npm_config_audit: baseEnv['npm_config_audit'] ?? 'false',
YARN_ENABLE_INTERACTIVE: baseEnv['YARN_ENABLE_INTERACTIVE'] ?? 'false',
DEBIAN_FRONTEND: baseEnv['DEBIAN_FRONTEND'] ?? 'noninteractive',
NEEDRESTART_MODE: baseEnv['NEEDRESTART_MODE'] ?? 'a',
GIT_TERMINAL_PROMPT: baseEnv['GIT_TERMINAL_PROMPT'] ?? '0',
GH_PROMPT_DISABLED: baseEnv['GH_PROMPT_DISABLED'] ?? '1',
GCM_INTERACTIVE: baseEnv['GCM_INTERACTIVE'] ?? 'never',
PIP_DISABLE_PIP_VERSION_CHECK:
baseEnv['PIP_DISABLE_PIP_VERSION_CHECK'] ?? '1',
CI: sanitizedEnv['CI'],
npm_config_yes: sanitizedEnv['npm_config_yes'],
npm_config_fund: sanitizedEnv['npm_config_fund'],
npm_config_audit: sanitizedEnv['npm_config_audit'],
YARN_ENABLE_INTERACTIVE: sanitizedEnv['YARN_ENABLE_INTERACTIVE'],
DEBIAN_FRONTEND: sanitizedEnv['DEBIAN_FRONTEND'],
NEEDRESTART_MODE: sanitizedEnv['NEEDRESTART_MODE'],
GIT_TERMINAL_PROMPT: sanitizedEnv['GIT_TERMINAL_PROMPT'],
GH_PROMPT_DISABLED: sanitizedEnv['GH_PROMPT_DISABLED'],
GCM_INTERACTIVE: sanitizedEnv['GCM_INTERACTIVE'],
PIP_DISABLE_PIP_VERSION_CHECK:
sanitizedEnv['PIP_DISABLE_PIP_VERSION_CHECK'],
References
  1. Rely on the schema as the single source of truth for configuration defaults, avoiding redundant nullish coalescing operators.
  2. Use a sanitized environment for variable expansion to prevent extensions from bypassing environment variable redaction.

});
}

// 3. Prepare Sandboxed Command
const sandboxedCommand = await sandboxManager.prepareCommand({
command: resolvedExecutable,
Expand Down
53 changes: 53 additions & 0 deletions packages/core/src/tools/shell.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ import {
} from './shell.js';
import { debugLogger } from '../index.js';
import { type Config } from '../config/config.js';
import { ApprovalMode } from '../policy/types.js';
import { NoopSandboxManager } from '../services/sandboxManager.js';
import {
type ShellExecutionResult,
Expand Down Expand Up @@ -898,6 +899,58 @@ EOF`;
await promise;
});
});

describe('PTY routing vs Full Access (YOLO)', () => {
it('should enable PTY when interactive shell is on and approvalMode is not YOLO', async () => {
vi.mocked(mockConfig.getEnableInteractiveShell).mockReturnValue(true);
vi.mocked(mockConfig.getApprovalMode).mockReturnValue(
ApprovalMode.DEFAULT,
);

const invocation = shellTool.build({ command: 'echo hi' });
const promise = invocation.execute({ abortSignal: mockAbortSignal });
resolveShellExecution();
await promise;

const call = mockShellExecutionService.mock.calls.at(-1);
// 5th positional arg (index 4) is shouldUseNodePty.
expect(call?.[4]).toBe(true);
});

it('should skip PTY in Full Access (YOLO) so sudo cannot prompt for a password', async () => {
// PTY would give sudo a real TTY and let it hang forever waiting for
// a password. The child_process fallback uses stdio:['ignore', ...]
// so sudo fails fast with "a password is required" (matches Codex's
// Stdio::null() in codex-rs/utils/pty/src/pipe.rs:144).
vi.mocked(mockConfig.getEnableInteractiveShell).mockReturnValue(true);
vi.mocked(mockConfig.getApprovalMode).mockReturnValue(
ApprovalMode.YOLO,
);

const invocation = shellTool.build({ command: 'sudo apt update' });
const promise = invocation.execute({ abortSignal: mockAbortSignal });
resolveShellExecution();
await promise;

const call = mockShellExecutionService.mock.calls.at(-1);
expect(call?.[4]).toBe(false);
});

it('should still skip PTY when interactive shell is disabled, regardless of approvalMode', async () => {
vi.mocked(mockConfig.getEnableInteractiveShell).mockReturnValue(false);
vi.mocked(mockConfig.getApprovalMode).mockReturnValue(
ApprovalMode.DEFAULT,
);

const invocation = shellTool.build({ command: 'echo hi' });
const promise = invocation.execute({ abortSignal: mockAbortSignal });
resolveShellExecution();
await promise;

const call = mockShellExecutionService.mock.calls.at(-1);
expect(call?.[4]).toBe(false);
});
});
});

describe('shouldConfirmExecute', () => {
Expand Down
9 changes: 8 additions & 1 deletion packages/core/src/tools/shell.ts
Original file line number Diff line number Diff line change
Expand Up @@ -651,11 +651,18 @@ export class ShellToolInvocation extends BaseToolInvocation<
}
},
combinedController.signal,
this.context.config.getEnableInteractiveShell(),
// In Full Access (YOLO) skip the PTY: a real TTY lets `sudo` and
// similar prompt for a password indefinitely. The child_process
// fallback uses stdio:['ignore', ...] so sudo fails fast with
// "a password is required" (matches Codex's Stdio::null() in
// codex-rs/utils/pty/src/pipe.rs:144).
this.context.config.getEnableInteractiveShell() &&
this.context.config.getApprovalMode() !== ApprovalMode.YOLO,
{
...shellExecutionConfig,
sessionId: this.context.config?.getSessionId?.() ?? 'default',
pager: 'cat',
approvalMode: this.context.config.getApprovalMode(),
sanitizationConfig:
shellExecutionConfig?.sanitizationConfig ??
this.context.config.sanitizationConfig,
Expand Down
Loading