diff --git a/codex-cli/scripts/build_container.sh b/codex-cli/scripts/build_container.sh index fd4c8f5a86..d4d29f6b34 100755 --- a/codex-cli/scripts/build_container.sh +++ b/codex-cli/scripts/build_container.sh @@ -8,9 +8,9 @@ pushd "$SCRIPT_DIR/.." >> /dev/null || { echo "Error: Failed to change directory to $SCRIPT_DIR/.." exit 1 } -npm install -npm run build +pnpm install +pnpm run build rm -rf ./dist/openai-codex-*.tgz -npm pack --pack-destination ./dist +pnpm pack --pack-destination ./dist mv ./dist/openai-codex-*.tgz ./dist/codex.tgz docker build -t codex -f "./Dockerfile" . diff --git a/codex-cli/src/components/chat/terminal-chat-input.tsx b/codex-cli/src/components/chat/terminal-chat-input.tsx index aad2e6b3c3..070f08f0ca 100644 --- a/codex-cli/src/components/chat/terminal-chat-input.tsx +++ b/codex-cli/src/components/chat/terminal-chat-input.tsx @@ -470,15 +470,8 @@ export default function TerminalChatInput({ setInput(""); try { - // Dynamically import dependencies to avoid unnecessary bundle size - const [{ default: open }, os] = await Promise.all([ - import("open"), - import("node:os"), - ]); - - // Lazy import CLI_VERSION to avoid circular deps + const os = await import("node:os"); const { CLI_VERSION } = await import("../../utils/session.js"); - const { buildBugReportUrl } = await import( "../../utils/bug-report.js" ); @@ -492,10 +485,6 @@ export default function TerminalChatInput({ .join(" | "), }); - // Open the URL in the user's default browser - await open(url, { wait: false }); - - // Inform the user in the chat history setItems((prev) => [ ...prev, { @@ -505,13 +494,13 @@ export default function TerminalChatInput({ content: [ { type: "input_text", - text: "📋 Opened browser to file a bug report. Please include any context that might help us fix the issue!", + text: `🔗 Bug report URL: ${url}`, }, ], }, ]); } catch (error) { - // If anything went wrong, notify the user + // If anything went wrong, notify the user. setItems((prev) => [ ...prev, { diff --git a/codex-cli/src/components/chat/terminal-chat.tsx b/codex-cli/src/components/chat/terminal-chat.tsx index e3638ac3f1..7f59c0b3c4 100644 --- a/codex-cli/src/components/chat/terminal-chat.tsx +++ b/codex-cli/src/components/chat/terminal-chat.tsx @@ -31,6 +31,7 @@ import DiffOverlay from "../diff-overlay.js"; import HelpOverlay from "../help-overlay.js"; import HistoryOverlay from "../history-overlay.js"; import ModelOverlay from "../model-overlay.js"; +import chalk from "chalk"; import { Box, Text } from "ink"; import { spawn } from "node:child_process"; import OpenAI from "openai"; @@ -575,7 +576,7 @@ export default function TerminalChat({ providers={config.providers} currentProvider={provider} hasLastResponse={Boolean(lastResponseId)} - onSelect={(newModel) => { + onSelect={(allModels, newModel) => { log( "TerminalChat: interruptAgent invoked – calling agent.cancel()", ); @@ -585,6 +586,20 @@ export default function TerminalChat({ agent?.cancel(); setLoading(false); + if (!allModels?.includes(newModel)) { + // eslint-disable-next-line no-console + console.error( + chalk.bold.red( + `Model "${chalk.yellow( + newModel, + )}" is not available for provider "${chalk.yellow( + provider, + )}".`, + ), + ); + return; + } + setModel(newModel); setLastResponseId((prev) => prev && newModel !== model ? null : prev, diff --git a/codex-cli/src/components/help-overlay.tsx b/codex-cli/src/components/help-overlay.tsx index 6eeffb9efb..d302f7551e 100644 --- a/codex-cli/src/components/help-overlay.tsx +++ b/codex-cli/src/components/help-overlay.tsx @@ -53,7 +53,8 @@ export default function HelpOverlay({ /clearhistory – clear command history - /bug – file a bug report with session log + /bug – generate a prefilled GitHub issue URL + with session log /diff – view working tree git diff diff --git a/codex-cli/src/components/model-overlay.tsx b/codex-cli/src/components/model-overlay.tsx index c9dde0e6b4..28b2575a71 100644 --- a/codex-cli/src/components/model-overlay.tsx +++ b/codex-cli/src/components/model-overlay.tsx @@ -19,7 +19,7 @@ type Props = { currentProvider?: string; hasLastResponse: boolean; providers?: Record; - onSelect: (model: string) => void; + onSelect: (allModels: Array, model: string) => void; onSelectProvider?: (provider: string) => void; onExit: () => void; }; @@ -153,7 +153,12 @@ export default function ModelOverlay({ } initialItems={items} currentValue={currentModel} - onSelect={onSelect} + onSelect={() => + onSelect( + items?.map((m) => m.value), + currentModel, + ) + } onExit={onExit} /> ); diff --git a/codex-cli/src/utils/slash-commands.ts b/codex-cli/src/utils/slash-commands.ts index b276c49135..4ccc3a9fc5 100644 --- a/codex-cli/src/utils/slash-commands.ts +++ b/codex-cli/src/utils/slash-commands.ts @@ -23,7 +23,10 @@ export const SLASH_COMMANDS: Array = [ { command: "/help", description: "Show list of commands" }, { command: "/model", description: "Open model selection panel" }, { command: "/approval", description: "Open approval mode selection panel" }, - { command: "/bug", description: "Generate a prefilled GitHub bug report" }, + { + command: "/bug", + description: "Generate a prefilled GitHub issue URL with session log", + }, { command: "/diff", description: diff --git a/codex-cli/tests/terminal-chat-model-selection.test.tsx b/codex-cli/tests/terminal-chat-model-selection.test.tsx new file mode 100644 index 0000000000..4e2bd5e999 --- /dev/null +++ b/codex-cli/tests/terminal-chat-model-selection.test.tsx @@ -0,0 +1,130 @@ +/* eslint-disable no-console */ +import { renderTui } from "./ui-test-helpers.js"; +import React from "react"; +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import chalk from "chalk"; +import ModelOverlay from "src/components/model-overlay.js"; + +// Mock the necessary dependencies +vi.mock("../src/utils/logger/log.js", () => ({ + log: vi.fn(), +})); + +vi.mock("chalk", () => ({ + default: { + bold: { + red: vi.fn((msg) => `[bold-red]${msg}[/bold-red]`), + }, + yellow: vi.fn((msg) => `[yellow]${msg}[/yellow]`), + }, +})); + +describe("Model Selection Error Handling", () => { + // Create a console.error spy with proper typing + let consoleErrorSpy: ReturnType; + + beforeEach(() => { + consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + }); + + afterEach(() => { + vi.clearAllMocks(); + consoleErrorSpy.mockRestore(); + }); + + it("should display error with chalk formatting when selecting unavailable model", () => { + // Setup + const allModels = ["gpt-4", "gpt-3.5-turbo"]; + const currentModel = "gpt-4"; + const unavailableModel = "gpt-invalid"; + const currentProvider = "openai"; + + renderTui( + { + if (!models?.includes(newModel)) { + console.error( + chalk.bold.red( + `Model "${chalk.yellow( + newModel, + )}" is not available for provider "${chalk.yellow( + currentProvider, + )}".`, + ), + ); + return; + } + }} + onSelectProvider={() => {}} + onExit={() => {}} + />, + ); + + const onSelectHandler = vi.fn((models, newModel) => { + if (!models?.includes(newModel)) { + console.error( + chalk.bold.red( + `Model "${chalk.yellow( + newModel, + )}" is not available for provider "${chalk.yellow( + currentProvider, + )}".`, + ), + ); + return; + } + }); + + onSelectHandler(allModels, unavailableModel); + + expect(consoleErrorSpy).toHaveBeenCalled(); + expect(chalk.bold.red).toHaveBeenCalled(); + expect(chalk.yellow).toHaveBeenCalledWith(unavailableModel); + expect(chalk.yellow).toHaveBeenCalledWith(currentProvider); + + expect(consoleErrorSpy).toHaveBeenCalledWith( + `[bold-red]Model "[yellow]${unavailableModel}[/yellow]" is not available for provider "[yellow]${currentProvider}[/yellow]".[/bold-red]`, + ); + }); + + it("should not proceed with model change when model is unavailable", () => { + const mockSetModel = vi.fn(); + const mockSetLastResponseId = vi.fn(); + const mockSaveConfig = vi.fn(); + const mockSetItems = vi.fn(); + const mockSetOverlayMode = vi.fn(); + + const onSelectHandler = vi.fn((allModels, newModel) => { + if (!allModels?.includes(newModel)) { + console.error( + chalk.bold.red( + `Model "${chalk.yellow( + newModel, + )}" is not available for provider "${chalk.yellow("openai")}".`, + ), + ); + return; + } + + mockSetModel(newModel); + mockSetLastResponseId(null); + mockSaveConfig({}); + mockSetItems((prev: Array) => [...prev, {}]); + mockSetOverlayMode("none"); + }); + + onSelectHandler(["gpt-4", "gpt-3.5-turbo"], "gpt-invalid"); + + expect(mockSetModel).not.toHaveBeenCalled(); + expect(mockSetLastResponseId).not.toHaveBeenCalled(); + expect(mockSaveConfig).not.toHaveBeenCalled(); + expect(mockSetItems).not.toHaveBeenCalled(); + expect(mockSetOverlayMode).not.toHaveBeenCalled(); + + expect(consoleErrorSpy).toHaveBeenCalled(); + }); +}); diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 74e466789e..e57d3bbf07 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -868,6 +868,7 @@ async fn handle_function_call( sandbox_type, &roots_snapshot, sess.ctrl_c.clone(), + sess.sandbox_policy, ) .await; @@ -952,11 +953,14 @@ async fn handle_function_call( let retry_roots = { sess.writable_roots.lock().unwrap().clone() }; + // This is an escalated retry; the policy will not be + // examined and the sandbox has been set to `None`. let retry_output_result = process_exec_tool_call( params.clone(), SandboxType::None, &retry_roots, sess.ctrl_c.clone(), + sess.sandbox_policy, ) .await; diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index 1a92c8adc3..ae83dc84e7 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -15,8 +15,10 @@ use tokio::sync::Notify; use crate::error::CodexErr; use crate::error::Result; use crate::error::SandboxErr; +use crate::protocol::SandboxPolicy; /// Maximum we keep for each stream (100 KiB). +/// TODO(ragona) this should be reduced const MAX_STREAM_OUTPUT: usize = 100 * 1024; const DEFAULT_TIMEOUT_MS: u64 = 10_000; @@ -55,8 +57,9 @@ async fn exec_linux( params: ExecParams, writable_roots: &[PathBuf], ctrl_c: Arc, + sandbox_policy: SandboxPolicy, ) -> Result { - crate::linux::exec_linux(params, writable_roots, ctrl_c).await + crate::linux::exec_linux(params, writable_roots, ctrl_c, sandbox_policy).await } #[cfg(not(target_os = "linux"))] @@ -64,6 +67,7 @@ async fn exec_linux( _params: ExecParams, _writable_roots: &[PathBuf], _ctrl_c: Arc, + _sandbox_policy: SandboxPolicy, ) -> Result { Err(CodexErr::Io(io::Error::new( io::ErrorKind::InvalidInput, @@ -76,6 +80,7 @@ pub async fn process_exec_tool_call( sandbox_type: SandboxType, writable_roots: &[PathBuf], ctrl_c: Arc, + sandbox_policy: SandboxPolicy, ) -> Result { let start = Instant::now(); @@ -98,7 +103,9 @@ pub async fn process_exec_tool_call( ) .await } - SandboxType::LinuxSeccomp => exec_linux(params, writable_roots, ctrl_c).await, + SandboxType::LinuxSeccomp => { + exec_linux(params, writable_roots, ctrl_c, sandbox_policy).await + } }; let duration = start.elapsed(); match raw_output_result { @@ -199,9 +206,17 @@ pub async fn exec( if let Some(dir) = &workdir { cmd.current_dir(dir); } - cmd.stdout(Stdio::piped()).stderr(Stdio::piped()); - cmd.kill_on_drop(true); - cmd.spawn()? + + // Do not create a file descriptor for stdin because otherwise some + // commands may hang forever waiting for input. For example, ripgrep has + // a heuristic where it may try to read from stdin as explained here: + // https://github.com/BurntSushi/ripgrep/blob/e2362d4d5185d02fa857bf381e7bd52e66fafc73/crates/core/flags/hiargs.rs#L1101-L1103 + cmd.stdin(Stdio::null()); + + cmd.stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .kill_on_drop(true) + .spawn()? }; let stdout_handle = tokio::spawn(read_capped( diff --git a/codex-rs/core/src/linux.rs b/codex-rs/core/src/linux.rs index f2dd9e6b96..61711c46bb 100644 --- a/codex-rs/core/src/linux.rs +++ b/codex-rs/core/src/linux.rs @@ -9,6 +9,7 @@ use crate::error::SandboxErr; use crate::exec::exec; use crate::exec::ExecParams; use crate::exec::RawExecToolCallOutput; +use crate::protocol::SandboxPolicy; use landlock::Access; use landlock::AccessFs; @@ -33,6 +34,7 @@ pub async fn exec_linux( params: ExecParams, writable_roots: &[PathBuf], ctrl_c: Arc, + sandbox_policy: SandboxPolicy, ) -> Result { // Allow READ on / // Allow WRITE on /dev/null @@ -47,34 +49,12 @@ pub async fn exec_linux( .expect("Failed to create runtime"); rt.block_on(async { - let abi = ABI::V5; - let access_rw = AccessFs::from_all(abi); - let access_ro = AccessFs::from_read(abi); - - let mut ruleset = Ruleset::default() - .set_compatibility(CompatLevel::BestEffort) - .handle_access(access_rw)? - .create()? - .add_rules(landlock::path_beneath_rules(&["/"], access_ro))? - .add_rules(landlock::path_beneath_rules(&["/dev/null"], access_rw))? - .set_no_new_privs(true); - - if !writable_roots_copy.is_empty() { - ruleset = ruleset.add_rules(landlock::path_beneath_rules( - &writable_roots_copy, - access_rw, - ))?; + if sandbox_policy.is_network_restricted() { + install_network_seccomp_filter_on_current_thread()?; } - let status = ruleset.restrict_self()?; - - // TODO(wpt): Probably wanna expand this more generically and not warn every time. - if status.ruleset == landlock::RulesetStatus::NotEnforced { - return Err(CodexErr::Sandbox(SandboxErr::LandlockRestrict)); - } - - if let Err(e) = install_network_seccomp_filter() { - return Err(CodexErr::Sandbox(e)); + if sandbox_policy.is_file_write_restricted() { + install_filesystem_landlock_rules_on_current_thread(writable_roots_copy)?; } exec(params, ctrl_c_copy).await @@ -92,7 +72,33 @@ pub async fn exec_linux( } } -fn install_network_seccomp_filter() -> std::result::Result<(), SandboxErr> { +fn install_filesystem_landlock_rules_on_current_thread(writable_roots: Vec) -> Result<()> { + let abi = ABI::V5; + let access_rw = AccessFs::from_all(abi); + let access_ro = AccessFs::from_read(abi); + + let mut ruleset = Ruleset::default() + .set_compatibility(CompatLevel::BestEffort) + .handle_access(access_rw)? + .create()? + .add_rules(landlock::path_beneath_rules(&["/"], access_ro))? + .add_rules(landlock::path_beneath_rules(&["/dev/null"], access_rw))? + .set_no_new_privs(true); + + if !writable_roots.is_empty() { + ruleset = ruleset.add_rules(landlock::path_beneath_rules(&writable_roots, access_rw))?; + } + + let status = ruleset.restrict_self()?; + + if status.ruleset == landlock::RulesetStatus::NotEnforced { + return Err(CodexErr::Sandbox(SandboxErr::LandlockRestrict)); + } + + Ok(()) +} + +fn install_network_seccomp_filter_on_current_thread() -> std::result::Result<(), SandboxErr> { // Build rule map. let mut rules: BTreeMap> = BTreeMap::new(); @@ -156,6 +162,7 @@ mod tests_linux { use crate::exec::process_exec_tool_call; use crate::exec::ExecParams; use crate::exec::SandboxType; + use crate::protocol::SandboxPolicy; use std::sync::Arc; use tempfile::NamedTempFile; use tokio::sync::Notify; @@ -172,6 +179,7 @@ mod tests_linux { SandboxType::LinuxSeccomp, writable_roots, Arc::new(Notify::new()), + SandboxPolicy::NetworkAndFileWriteRestricted, ) .await .unwrap(); @@ -238,6 +246,7 @@ mod tests_linux { SandboxType::LinuxSeccomp, &[], Arc::new(Notify::new()), + SandboxPolicy::NetworkRestricted, ) .await; diff --git a/codex-rs/core/src/protocol.rs b/codex-rs/core/src/protocol.rs index d1975ae847..42c8478e6b 100644 --- a/codex-rs/core/src/protocol.rs +++ b/codex-rs/core/src/protocol.rs @@ -100,6 +100,30 @@ pub enum SandboxPolicy { DangerousNoRestrictions, } +impl SandboxPolicy { + pub fn is_dangerous(&self) -> bool { + match self { + SandboxPolicy::NetworkRestricted => false, + SandboxPolicy::FileWriteRestricted => false, + SandboxPolicy::NetworkAndFileWriteRestricted => false, + SandboxPolicy::DangerousNoRestrictions => true, + } + } + + pub fn is_network_restricted(&self) -> bool { + matches!( + self, + SandboxPolicy::NetworkRestricted | SandboxPolicy::NetworkAndFileWriteRestricted + ) + } + + pub fn is_file_write_restricted(&self) -> bool { + matches!( + self, + SandboxPolicy::FileWriteRestricted | SandboxPolicy::NetworkAndFileWriteRestricted + ) + } +} /// User input #[non_exhaustive] #[derive(Debug, Clone, Deserialize, Serialize)]