From f8fcaaaf6f28c769b452c1fdeb4a62bd14a9415f Mon Sep 17 00:00:00 2001 From: easong-openai Date: Tue, 29 Jul 2025 10:06:05 -0700 Subject: [PATCH] Relative instruction file (#1722) Passing in an instruction file with a bad path led to silent failures, also instruction relative paths were handled in an unintuitive fashion. --- codex-rs/core/src/config.rs | 54 ++++++++++++++++--- codex-rs/core/tests/cli_stream.rs | 90 +++++++++++++++++++++++++++++++ codex-rs/exec/src/lib.rs | 28 +++++----- 3 files changed, 150 insertions(+), 22 deletions(-) diff --git a/codex-rs/core/src/config.rs b/codex-rs/core/src/config.rs index 2dfd3e55..57027bd0 100644 --- a/codex-rs/core/src/config.rs +++ b/codex-rs/core/src/config.rs @@ -465,9 +465,14 @@ impl Config { let experimental_resume = cfg.experimental_resume; - let base_instructions = base_instructions.or(Self::get_base_instructions( + // Load base instructions override from a file if specified. If the + // path is relative, resolve it against the effective cwd so the + // behaviour matches other path-like config values. + let file_base_instructions = Self::get_base_instructions( cfg.experimental_instructions_file.as_ref(), - )); + &resolved_cwd, + )?; + let base_instructions = base_instructions.or(file_base_instructions); let config = Self { model, @@ -539,13 +544,46 @@ impl Config { }) } - fn get_base_instructions(path: Option<&PathBuf>) -> Option { - let path = path.as_ref()?; + fn get_base_instructions( + path: Option<&PathBuf>, + cwd: &Path, + ) -> std::io::Result> { + let p = match path.as_ref() { + None => return Ok(None), + Some(p) => p, + }; - std::fs::read_to_string(path) - .ok() - .map(|s| s.trim().to_string()) - .filter(|s| !s.is_empty()) + // Resolve relative paths against the provided cwd to make CLI + // overrides consistent regardless of where the process was launched + // from. + let full_path = if p.is_relative() { + cwd.join(p) + } else { + p.to_path_buf() + }; + + let contents = std::fs::read_to_string(&full_path).map_err(|e| { + std::io::Error::new( + e.kind(), + format!( + "failed to read experimental instructions file {}: {e}", + full_path.display() + ), + ) + })?; + + let s = contents.trim().to_string(); + if s.is_empty() { + Err(std::io::Error::new( + std::io::ErrorKind::InvalidData, + format!( + "experimental instructions file is empty: {}", + full_path.display() + ), + )) + } else { + Ok(Some(s)) + } } } diff --git a/codex-rs/core/tests/cli_stream.rs b/codex-rs/core/tests/cli_stream.rs index 4694ba85..0ab7bd0b 100644 --- a/codex-rs/core/tests/cli_stream.rs +++ b/codex-rs/core/tests/cli_stream.rs @@ -81,6 +81,96 @@ async fn chat_mode_stream_cli() { server.verify().await; } +/// Verify that passing `-c experimental_instructions_file=...` to the CLI +/// overrides the built-in base instructions by inspecting the request body +/// received by a mock OpenAI Responses endpoint. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn exec_cli_applies_experimental_instructions_file() { + if std::env::var(CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR).is_ok() { + println!( + "Skipping test because it cannot execute when network is disabled in a Codex sandbox." + ); + return; + } + + // Start mock server which will capture the request and return a minimal + // SSE stream for a single turn. + let server = MockServer::start().await; + let sse = concat!( + "data: {\"type\":\"response.created\",\"response\":{}}\n\n", + "data: {\"type\":\"response.completed\",\"response\":{\"id\":\"r1\"}}\n\n" + ); + Mock::given(method("POST")) + .and(path("/v1/responses")) + .respond_with( + ResponseTemplate::new(200) + .insert_header("content-type", "text/event-stream") + .set_body_raw(sse, "text/event-stream"), + ) + .expect(1) + .mount(&server) + .await; + + // Create a temporary instructions file with a unique marker we can assert + // appears in the outbound request payload. + let custom = TempDir::new().unwrap(); + let marker = "cli-experimental-instructions-marker"; + let custom_path = custom.path().join("instr.md"); + std::fs::write(&custom_path, marker).unwrap(); + let custom_path_str = custom_path.to_string_lossy().replace('\\', "/"); + + // Build a provider override that points at the mock server and instructs + // Codex to use the Responses API with the dummy env var. + let provider_override = format!( + "model_providers.mock={{ name = \"mock\", base_url = \"{}/v1\", env_key = \"PATH\", wire_api = \"responses\" }}", + server.uri() + ); + + let home = TempDir::new().unwrap(); + let mut cmd = AssertCommand::new("cargo"); + cmd.arg("run") + .arg("-p") + .arg("codex-cli") + .arg("--quiet") + .arg("--") + .arg("exec") + .arg("--skip-git-repo-check") + .arg("-c") + .arg(&provider_override) + .arg("-c") + .arg("model_provider=\"mock\"") + .arg("-c") + .arg(format!( + "experimental_instructions_file=\"{custom_path_str}\"" + )) + .arg("-C") + .arg(env!("CARGO_MANIFEST_DIR")) + .arg("hello?\n"); + cmd.env("CODEX_HOME", home.path()) + .env("OPENAI_API_KEY", "dummy") + .env("OPENAI_BASE_URL", format!("{}/v1", server.uri())); + + let output = cmd.output().unwrap(); + println!("Status: {}", output.status); + println!("Stdout:\n{}", String::from_utf8_lossy(&output.stdout)); + println!("Stderr:\n{}", String::from_utf8_lossy(&output.stderr)); + assert!(output.status.success()); + + // Inspect the captured request and verify our custom base instructions were + // included in the `instructions` field. + let request = &server.received_requests().await.unwrap()[0]; + let body = request.body_json::().unwrap(); + let instructions = body + .get("instructions") + .and_then(|v| v.as_str()) + .unwrap_or_default() + .to_string(); + assert!( + instructions.contains(marker), + "instructions did not contain custom marker; got: {instructions}" + ); +} + /// Tests streaming responses through the CLI using a local SSE fixture file. /// This test: /// 1. Uses a pre-recorded SSE response fixture instead of a live server diff --git a/codex-rs/exec/src/lib.rs b/codex-rs/exec/src/lib.rs index f966d200..cf2f2bd6 100644 --- a/codex-rs/exec/src/lib.rs +++ b/codex-rs/exec/src/lib.rs @@ -92,6 +92,20 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option) -> any ), }; + // TODO(mbolin): Take a more thoughtful approach to logging. + let default_level = "error"; + let _ = tracing_subscriber::fmt() + // Fallback to the `default_level` log filter if the environment + // variable is not set _or_ contains an invalid value + .with_env_filter( + EnvFilter::try_from_default_env() + .or_else(|_| EnvFilter::try_new(default_level)) + .unwrap_or_else(|_| EnvFilter::new(default_level)), + ) + .with_ansi(stderr_with_ansi) + .with_writer(std::io::stderr) + .try_init(); + let sandbox_mode = if full_auto { Some(SandboxMode::WorkspaceWrite) } else if dangerously_bypass_approvals_and_sandbox { @@ -142,20 +156,6 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option) -> any std::process::exit(1); } - // TODO(mbolin): Take a more thoughtful approach to logging. - let default_level = "error"; - let _ = tracing_subscriber::fmt() - // Fallback to the `default_level` log filter if the environment - // variable is not set _or_ contains an invalid value - .with_env_filter( - EnvFilter::try_from_default_env() - .or_else(|_| EnvFilter::try_new(default_level)) - .unwrap_or_else(|_| EnvFilter::new(default_level)), - ) - .with_ansi(stderr_with_ansi) - .with_writer(std::io::stderr) - .try_init(); - let CodexConversation { codex: codex_wrapper, session_configured,