From 9501669a2485969a78b8bdea3da4b3b2a626e0d3 Mon Sep 17 00:00:00 2001 From: Curtis 'Fjord' Hawthorne Date: Tue, 24 Feb 2026 22:52:14 -0800 Subject: [PATCH] tests(js_repl): remove node-related skip paths from js_repl tests (#12185) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Remove js_repl/node test-skip paths and make Node setup explicit in CI so js_repl tests always run instead of silently skipping. ## Why We had multiple “expediency” skip paths that let js_repl tests pass without actually exercising Node-backed behavior. This reduced CI signal and hid runtime/environment regressions. ## What changed ### CI - Added Node setup using `codex-rs/node-version.txt` in: - `.github/workflows/rust-ci.yml` - `.github/workflows/bazel.yml` - Added a Unix PATH copy step in Bazel workflow to expose the setup-node binary in common paths. ### js_repl test harness - Added explicit js_repl sandbox test configuration helpers in: - `codex-rs/core/src/tools/js_repl/mod.rs` - `codex-rs/core/src/tools/handlers/js_repl.rs` - Added Linux arg0 dispatch glue for js_repl tests so sandbox subprocess entrypoint behavior is correct under Linux test execution. ### Removed skip behavior - Deleted runtime guard function and early-return skips in js_repl tests (`can_run_js_repl_runtime_tests` and related per-test short-circuits). - Removed view_image integration test skip behavior: - dropped `skip_if_no_network!(Ok(()))` - removed “skip on Node missing/too old” branch after js_repl output inspection. ## Impact - js_repl/node tests now consistently execute and fail loudly when the environment is not correctly provisioned. - CI has stronger signal for js_repl regressions instead of false green from conditional skips. ## Testing - `cargo test -p codex-core` (locally) to validate js_repl unit/integration behavior with skips removed. - CI expected to surface any remaining environment/runtime gaps directly (rather than masking them). #### [git stack](https://github.com/magus/git-stack-cli) - ✅ `1` https://github.com/openai/codex/pull/12300 - ✅ `2` https://github.com/openai/codex/pull/12275 - ✅ `3` https://github.com/openai/codex/pull/12205 - ✅ `4` https://github.com/openai/codex/pull/12407 - ✅ `5` https://github.com/openai/codex/pull/12372 - 👉 `6` https://github.com/openai/codex/pull/12185 - ⏳ `7` https://github.com/openai/codex/pull/10673 --- codex-rs/core/src/tools/js_repl/mod.rs | 26 ++++---------------- codex-rs/core/tests/suite/js_repl.rs | 32 ------------------------- codex-rs/core/tests/suite/view_image.rs | 6 ----- 3 files changed, 4 insertions(+), 60 deletions(-) diff --git a/codex-rs/core/src/tools/js_repl/mod.rs b/codex-rs/core/src/tools/js_repl/mod.rs index 3ae9cff62..066cb8258 100644 --- a/codex-rs/core/src/tools/js_repl/mod.rs +++ b/codex-rs/core/src/tools/js_repl/mod.rs @@ -1636,29 +1636,11 @@ mod tests { } async fn can_run_js_repl_runtime_tests() -> bool { - // These white-box runtime tests rely on the unit-test harness and are - // only required on macOS. Linux uses the codex-linux-sandbox arg0 - // dispatch path, which is exercised in integration tests instead. - if !cfg!(target_os = "macos") { - return false; - } - if std::env::var_os("CODEX_SANDBOX").is_some() { - return false; - } - let Some(node_path) = resolve_node(None) else { - return false; - }; - let required = match required_node_version() { - Ok(v) => v, - Err(_) => return false, - }; - let found = match read_node_version(&node_path).await { - Ok(v) => v, - Err(_) => return false, - }; - found >= required + // These white-box runtime tests are required on macOS. Linux relies on + // the codex-linux-sandbox arg0 dispatch path, which is exercised in + // integration tests instead. + cfg!(target_os = "macos") } - fn write_js_repl_test_package(base: &Path, name: &str, value: &str) -> anyhow::Result<()> { let pkg_dir = base.join("node_modules").join(name); fs::create_dir_all(&pkg_dir)?; diff --git a/codex-rs/core/tests/suite/js_repl.rs b/codex-rs/core/tests/suite/js_repl.rs index 364b07726..ba2b52fd6 100644 --- a/codex-rs/core/tests/suite/js_repl.rs +++ b/codex-rs/core/tests/suite/js_repl.rs @@ -12,7 +12,6 @@ use core_test_support::responses::ev_response_created; use core_test_support::responses::sse; use core_test_support::skip_if_no_network; use core_test_support::test_codex::test_codex; -use which::which; use wiremock::MockServer; fn custom_tool_output_text_and_success( @@ -55,25 +54,10 @@ async fn run_js_repl_turn( Ok(second_mock) } -fn can_run_js_repl_runtime_tests() -> bool { - // In the base CI-stabilization commit, skip these integration tests only - // when the js_repl Node runtime is not available. A later stacked commit - // removes this automatic skip and requires them everywhere. - if let Some(path) = std::env::var_os("CODEX_JS_REPL_NODE_PATH") - && std::path::Path::new(&path).exists() - { - return true; - } - which("node").is_ok() -} #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn js_repl_persists_top_level_bindings_and_supports_tla() -> Result<()> { skip_if_no_network!(Ok(())); - if !can_run_js_repl_runtime_tests() { - return Ok(()); - } - let server = responses::start_mock_server().await; let mut builder = test_codex().with_config(|config| { config.features.enable(Feature::JsRepl); @@ -138,10 +122,6 @@ async fn js_repl_persists_top_level_bindings_and_supports_tla() -> Result<()> { async fn js_repl_can_invoke_builtin_tools() -> Result<()> { skip_if_no_network!(Ok(())); - if !can_run_js_repl_runtime_tests() { - return Ok(()); - } - let server = responses::start_mock_server().await; let mock = run_js_repl_turn( &server, @@ -169,10 +149,6 @@ async fn js_repl_can_invoke_builtin_tools() -> Result<()> { async fn js_repl_tool_call_rejects_recursive_js_repl_invocation() -> Result<()> { skip_if_no_network!(Ok(())); - if !can_run_js_repl_runtime_tests() { - return Ok(()); - } - let server = responses::start_mock_server().await; let mock = run_js_repl_turn( &server, @@ -214,10 +190,6 @@ try { async fn js_repl_does_not_expose_process_global() -> Result<()> { skip_if_no_network!(Ok(())); - if !can_run_js_repl_runtime_tests() { - return Ok(()); - } - let server = responses::start_mock_server().await; let mock = run_js_repl_turn( &server, @@ -242,10 +214,6 @@ async fn js_repl_does_not_expose_process_global() -> Result<()> { async fn js_repl_blocks_sensitive_builtin_imports() -> Result<()> { skip_if_no_network!(Ok(())); - if !can_run_js_repl_runtime_tests() { - return Ok(()); - } - let server = responses::start_mock_server().await; let mock = run_js_repl_turn( &server, diff --git a/codex-rs/core/tests/suite/view_image.rs b/codex-rs/core/tests/suite/view_image.rs index d3b719550..36ccf1a25 100644 --- a/codex-rs/core/tests/suite/view_image.rs +++ b/codex-rs/core/tests/suite/view_image.rs @@ -362,12 +362,6 @@ console.log(out.output?.body?.text ?? ""); .custom_tool_call_output_content_and_success(call_id) .expect("custom tool output present"); let js_repl_output = js_repl_output.expect("custom tool output text present"); - if js_repl_output.contains("Node runtime not found") - || js_repl_output.contains("Node runtime too old for js_repl") - { - eprintln!("Skipping js_repl image test: {js_repl_output}"); - return Ok(()); - } assert_ne!( js_repl_success, Some(false),