Disable js_repl when Node is incompatible at startup (#12824)
## Summary - validate `js_repl` Node compatibility during session startup when the experiment is enabled - if Node is missing or too old, disable `js_repl` and `js_repl_tools_only` for the session before tools and instructions are built - surface that startup disablement to users through the existing startup warning flow instead of only logging it - reuse the same compatibility check in js_repl kernel startup so startup gating and runtime behavior stay aligned - add a regression test that verifies the warning is emitted and that the first advertised tool list omits `js_repl` and `js_repl_reset` when Node is incompatible ## Why Today `js_repl` can be advertised based only on the feature flag, then fail later when the kernel starts. That makes the available tool list inaccurate at the start of a conversation, and users do not get a clear explanation for why the tool is unavailable. This change makes tool availability reflect real startup checks, keeps the advertised tool set stable for the lifetime of the session, and gives users a visible warning when `js_repl` is disabled. ## Testing - `just fmt` - `cargo test -p codex-core --test all js_repl_is_not_advertised_when_startup_node_is_incompatible`
This commit is contained in:
parent
14116ade8d
commit
40ab71a985
3 changed files with 126 additions and 4 deletions
|
|
@ -251,6 +251,7 @@ use crate::tools::ToolRouter;
|
|||
use crate::tools::context::SharedTurnDiffTracker;
|
||||
use crate::tools::handlers::SEARCH_TOOL_BM25_TOOL_NAME;
|
||||
use crate::tools::js_repl::JsReplHandle;
|
||||
use crate::tools::js_repl::resolve_compatible_node;
|
||||
use crate::tools::network_approval::NetworkApprovalService;
|
||||
use crate::tools::network_approval::build_blocked_request_observer;
|
||||
use crate::tools::network_approval::build_network_policy_decider;
|
||||
|
|
@ -341,6 +342,18 @@ impl Codex {
|
|||
config.features.disable(Feature::Collab);
|
||||
}
|
||||
|
||||
if config.features.enabled(Feature::JsRepl)
|
||||
&& let Err(err) = resolve_compatible_node(config.js_repl_node_path.as_deref()).await
|
||||
{
|
||||
let message = format!(
|
||||
"Disabled `js_repl` for this session because the configured Node runtime is unavailable or incompatible. {err}"
|
||||
);
|
||||
warn!("{message}");
|
||||
config.features.disable(Feature::JsRepl);
|
||||
config.features.disable(Feature::JsReplToolsOnly);
|
||||
config.startup_warnings.push(message);
|
||||
}
|
||||
|
||||
let allowed_skills_for_implicit_invocation =
|
||||
loaded_skills.allowed_skills_for_implicit_invocation();
|
||||
let user_instructions =
|
||||
|
|
|
|||
|
|
@ -556,10 +556,7 @@ impl JsReplManager {
|
|||
turn: Arc<TurnContext>,
|
||||
thread_id: Option<ThreadId>,
|
||||
) -> Result<KernelState, String> {
|
||||
let node_path = resolve_node(self.node_path.as_deref()).ok_or_else(|| {
|
||||
"Node runtime not found; install Node or set CODEX_JS_REPL_NODE_PATH".to_string()
|
||||
})?;
|
||||
ensure_node_version(&node_path).await?;
|
||||
let node_path = resolve_compatible_node(self.node_path.as_deref()).await?;
|
||||
|
||||
let kernel_path = self
|
||||
.write_kernel_script()
|
||||
|
|
@ -1321,6 +1318,14 @@ async fn ensure_node_version(node_path: &Path) -> Result<(), String> {
|
|||
Ok(())
|
||||
}
|
||||
|
||||
pub(crate) async fn resolve_compatible_node(config_path: Option<&Path>) -> Result<PathBuf, String> {
|
||||
let node_path = resolve_node(config_path).ok_or_else(|| {
|
||||
"Node runtime not found; install Node or set CODEX_JS_REPL_NODE_PATH".to_string()
|
||||
})?;
|
||||
ensure_node_version(&node_path).await?;
|
||||
Ok(node_path)
|
||||
}
|
||||
|
||||
pub(crate) fn resolve_node(config_path: Option<&Path>) -> Option<PathBuf> {
|
||||
if let Some(path) = std::env::var_os("CODEX_JS_REPL_NODE_PATH") {
|
||||
let p = PathBuf::from(path);
|
||||
|
|
|
|||
|
|
@ -2,6 +2,7 @@
|
|||
|
||||
use anyhow::Result;
|
||||
use codex_core::features::Feature;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use core_test_support::responses;
|
||||
use core_test_support::responses::ResponseMock;
|
||||
use core_test_support::responses::ResponsesRequest;
|
||||
|
|
@ -12,6 +13,12 @@ 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 core_test_support::wait_for_event_match;
|
||||
use std::fs;
|
||||
#[cfg(unix)]
|
||||
use std::os::unix::fs::PermissionsExt;
|
||||
use std::path::Path;
|
||||
use tempfile::tempdir;
|
||||
use wiremock::MockServer;
|
||||
|
||||
fn custom_tool_output_text_and_success(
|
||||
|
|
@ -24,6 +31,45 @@ fn custom_tool_output_text_and_success(
|
|||
(output.unwrap_or_default(), success)
|
||||
}
|
||||
|
||||
fn tool_names(body: &serde_json::Value) -> Vec<String> {
|
||||
body["tools"]
|
||||
.as_array()
|
||||
.expect("tools array should be present")
|
||||
.iter()
|
||||
.map(|tool| {
|
||||
tool.get("name")
|
||||
.and_then(|value| value.as_str())
|
||||
.or_else(|| tool.get("type").and_then(|value| value.as_str()))
|
||||
.expect("tool should have a name or type")
|
||||
.to_string()
|
||||
})
|
||||
.collect()
|
||||
}
|
||||
|
||||
fn write_too_old_node_script(dir: &Path) -> Result<std::path::PathBuf> {
|
||||
#[cfg(windows)]
|
||||
{
|
||||
let path = dir.join("old-node.cmd");
|
||||
fs::write(&path, "@echo off\r\necho v0.0.1\r\n")?;
|
||||
Ok(path)
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
{
|
||||
let path = dir.join("old-node.sh");
|
||||
fs::write(&path, "#!/bin/sh\necho v0.0.1\n")?;
|
||||
let mut permissions = fs::metadata(&path)?.permissions();
|
||||
permissions.set_mode(0o755);
|
||||
fs::set_permissions(&path, permissions)?;
|
||||
Ok(path)
|
||||
}
|
||||
|
||||
#[cfg(not(any(unix, windows)))]
|
||||
{
|
||||
anyhow::bail!("unsupported platform for js_repl test fixture");
|
||||
}
|
||||
}
|
||||
|
||||
async fn run_js_repl_turn(
|
||||
server: &MockServer,
|
||||
prompt: &str,
|
||||
|
|
@ -54,6 +100,64 @@ async fn run_js_repl_turn(
|
|||
Ok(second_mock)
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn js_repl_is_not_advertised_when_startup_node_is_incompatible() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
if std::env::var_os("CODEX_JS_REPL_NODE_PATH").is_some() {
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
let server = responses::start_mock_server().await;
|
||||
let temp = tempdir()?;
|
||||
let old_node = write_too_old_node_script(temp.path())?;
|
||||
|
||||
let mut builder = test_codex().with_config(move |config| {
|
||||
config.features.enable(Feature::JsRepl);
|
||||
config.js_repl_node_path = Some(old_node);
|
||||
});
|
||||
let test = builder.build(&server).await?;
|
||||
let warning = wait_for_event_match(&test.codex, |event| match event {
|
||||
EventMsg::Warning(ev) if ev.message.contains("Disabled `js_repl` for this session") => {
|
||||
Some(ev.message.clone())
|
||||
}
|
||||
_ => None,
|
||||
})
|
||||
.await;
|
||||
assert!(
|
||||
warning.contains("Node runtime"),
|
||||
"warning should explain the Node compatibility issue: {warning}"
|
||||
);
|
||||
|
||||
let request_mock = responses::mount_sse_once(
|
||||
&server,
|
||||
sse(vec![
|
||||
ev_assistant_message("msg-1", "done"),
|
||||
ev_completed("resp-1"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
|
||||
test.submit_turn("hello").await?;
|
||||
|
||||
let body = request_mock.single_request().body_json();
|
||||
let tools = tool_names(&body);
|
||||
assert!(
|
||||
!tools.iter().any(|tool| tool == "js_repl"),
|
||||
"js_repl should be omitted when startup validation fails: {tools:?}"
|
||||
);
|
||||
assert!(
|
||||
!tools.iter().any(|tool| tool == "js_repl_reset"),
|
||||
"js_repl_reset should be omitted when startup validation fails: {tools:?}"
|
||||
);
|
||||
let instructions = body["instructions"].as_str().unwrap_or_default();
|
||||
assert!(
|
||||
!instructions.contains("## JavaScript REPL (Node)"),
|
||||
"startup instructions should not mention js_repl when it is disabled: {instructions}"
|
||||
);
|
||||
|
||||
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(()));
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue