fix: address PR review comments

- Copy fn pointer out of CUSTOM_MODULES lock before calling module
  declaration to prevent potential deadlock (spin::Mutex not re-entrant)
- Move cargo:rerun-if-env-changed=HYPERLIGHT_JS_RUNTIME_PATH before the
  match so Cargo tracks the env var even when unset; use canonical path
  for rerun-if-changed
- Lock down globalThis.console binding (non-writable/non-configurable)
  after Object.freeze to prevent handler code replacing it entirely
- Fix test doc comment: built-in overrides work except for require

Signed-off-by: Simon Davies <simongdavies@users.noreply.github.com>

Author: simongdavies

Cargo.toml

@@ -1,7 +1,7 @@
 [workspace]
 resolver = "2"
 members = ["src/hyperlight-js", "src/js-host-api", "src/hyperlight-js-runtime"]
-exclude = ["src/hyperlight-js-runtime/tests/fixtures/extended_runtime"]
+exclude = ["src/hyperlight-js-runtime/tests/fixtures"]
 
 [workspace.package]
 version = "0.1.1"

src/hyperlight-js-runtime/src/globals/freeze.rs

@@ -20,7 +20,8 @@ use rquickjs::Ctx;
 /// Called AFTER custom_globals! so extender crates can modify/extend
 /// globals first (e.g. adding console.warn/error/info/debug).
 ///
-/// Frozen: console (Object.freeze), print (non-writable).
+/// Frozen: console (Object.freeze + non-writable/non-configurable binding),
+///         print (non-writable/non-configurable binding).
 /// Already frozen: require (non-configurable from setup),
 ///                 String.bytesFrom (on frozen String constructor).
 pub fn setup(ctx: &Ctx<'_>) -> rquickjs::Result<()> {
@@ -29,6 +30,12 @@ pub fn setup(ctx: &Ctx<'_>) -> rquickjs::Result<()> {
         if (typeof globalThis.console === 'object') {
             Object.freeze(globalThis.console);
         }
+        if ('console' in globalThis) {
+            Object.defineProperty(globalThis, 'console', {
+                writable: false,
+                configurable: false
+            });
+        }
         if (typeof globalThis.print === 'function') {
             Object.defineProperty(globalThis, 'print', {
                 writable: false,

src/hyperlight-js-runtime/src/main.rs

@@ -31,5 +31,17 @@ hyperlight_js_runtime::custom_globals! {}
 // etc.) is provided by the lib's `guest` module.
 // The binary only needs to provide the native CLI entry point.
 
+// Force the guest module to be linked into the final hyperlight binary.
+// Without this, the linker may drop the guest module's object since nothing
+// in main.rs references the guest entrypoints directly.
+#[cfg(hyperlight)]
+unsafe extern "C" {
+    fn hyperlight_main();
+}
+
+#[cfg(hyperlight)]
+#[used]
+static _FORCE_GUEST_LINK: unsafe extern "C" fn() = hyperlight_main;
+
 #[cfg(not(hyperlight))]
 include!("main/native.rs");

src/hyperlight-js-runtime/src/modules/mod.rs

@@ -21,10 +21,14 @@ use rquickjs::module::ModuleDef;
 use rquickjs::{Ctx, Module, Result};
 use spin::{Lazy, Mutex};
 
-pub(crate) mod console;
-pub(crate) mod crypto;
-pub(crate) mod io;
-pub(crate) mod require;
+#[doc(hidden)]
+pub mod console;
+#[doc(hidden)]
+pub mod crypto;
+#[doc(hidden)]
+pub mod io;
+#[doc(hidden)]
+pub mod require;
 
 /// A function pointer type for declaring a native module.
 #[doc(hidden)]
@@ -115,7 +119,9 @@ pub struct NativeModuleLoader;
 impl Resolver for NativeModuleLoader {
     fn resolve(&mut self, _ctx: &Ctx<'_>, base: &str, name: &str) -> Result<String> {
         ensure_custom_modules_init();
-        if CUSTOM_MODULES.lock().contains_key(name) || BUILTIN_MODULES.contains_key(name) {
+        // Copy result out before dropping the lock to avoid holding it during resolution
+        let found_custom = CUSTOM_MODULES.lock().contains_key(name);
+        if found_custom || BUILTIN_MODULES.contains_key(name) {
             Ok(name.to_string())
         } else {
             Err(rquickjs::Error::new_resolving(base, name))
@@ -126,8 +132,12 @@ impl Resolver for NativeModuleLoader {
 impl Loader for NativeModuleLoader {
     fn load<'js>(&mut self, ctx: &Ctx<'js>, name: &str) -> Result<Module<'js>> {
         ensure_custom_modules_init();
-        // Check custom modules first
-        if let Some(decl) = CUSTOM_MODULES.lock().get(name) {
+        // Copy the fn pointer out while holding the lock, then drop the guard
+        // before calling the declaration. This avoids deadlock if decl()
+        // triggers a nested module load that tries to lock CUSTOM_MODULES
+        // (spin::Mutex is not re-entrant).
+        let custom_decl = CUSTOM_MODULES.lock().get(name).copied();
+        if let Some(decl) = custom_decl {
             return decl(ctx.clone(), name);
         }
         // Fall back to built-in modules

src/hyperlight-js-runtime/tests/fixtures/extended_runtime/Cargo.lock

@@ -50,6 +50,13 @@ fn main() -> anyhow::Result<()> {
     use std::{env, fs};
 
     let args: Vec<String> = env::args().collect();
+    if args.len() < 3 {
+        let program = args.first().map(|s| s.as_str()).unwrap_or("extended_runtime");
+        eprintln!("Usage: {program} <handler_file> <event_json>");
+        return Err(anyhow::anyhow!(
+            "missing required arguments: <handler_file> and <event_json>"
+        ));
+    }
     let file = std::path::PathBuf::from(&args[1]);
     let event = &args[2];
 
@@ -77,3 +84,13 @@ fn main() -> anyhow::Result<()> {
 
 // For hyperlight builds: the lib's `guest` module provides hyperlight_main,
 // guest_dispatch_function, and all plumbing. Nothing else needed here.
+
+// Force the guest module to be linked for hyperlight builds.
+#[cfg(hyperlight)]
+unsafe extern "C" {
+    fn hyperlight_main();
+}
+
+#[cfg(hyperlight)]
+#[used]
+static _FORCE_GUEST_LINK: unsafe extern "C" fn() = hyperlight_main;

src/hyperlight-js-runtime/tests/fixtures/extended_runtime/src/main.rs

@@ -20,7 +20,7 @@ limitations under the License.
 //! - `register_native_module` adds custom modules to the loader
 //! - Built-in modules always work
 //! - Custom modules can be imported from JS handlers
-//! - Built-in module names cannot be overridden (panics)
+//! - Overriding most built-in module names works; overriding `require` panics
 //! - The `native_modules!` macro generates correct `init_native_modules`
 //! - Full pipeline tests with the extended_runtime fixture binary
 
@@ -345,8 +345,30 @@ const EXTENDED_RUNTIME_MANIFEST: &str = concat!(
 );
 
 static EXTENDED_RUNTIME_BINARY: LazyLock<PathBuf> = LazyLock::new(|| {
+    let fixture_dir = PathBuf::from(EXTENDED_RUNTIME_MANIFEST)
+        .parent()
+        .unwrap()
+        .to_path_buf();
+    let target_dir = fixture_dir.join("target");
+
+    // Match the profile of the outer test build so the fixture binary
+    // lands in the same subdirectory we look for it in.
+    let profile = if cfg!(debug_assertions) {
+        "dev"
+    } else {
+        "release"
+    };
+
     let output = Command::new("cargo")
-        .args(["build", "--manifest-path", EXTENDED_RUNTIME_MANIFEST])
+        .args([
+            "build",
+            "--locked",
+            "--manifest-path",
+            EXTENDED_RUNTIME_MANIFEST,
+            "--target-dir",
+        ])
+        .arg(&target_dir)
+        .args(["--profile", profile])
         .output()
         .expect("Failed to run cargo build for extended-runtime fixture");
 
@@ -356,16 +378,14 @@ static EXTENDED_RUNTIME_BINARY: LazyLock<PathBuf> = LazyLock::new(|| {
         String::from_utf8_lossy(&output.stderr)
     );
 
-    let fixture_dir = PathBuf::from(EXTENDED_RUNTIME_MANIFEST)
-        .parent()
-        .unwrap()
-        .to_path_buf();
+    // Cargo outputs "dev" profile binaries to "debug" directory
+    let profile_dir = if profile == "dev" { "debug" } else { profile };
     let binary_name = if cfg!(windows) {
         "extended-runtime.exe"
     } else {
         "extended-runtime"
     };
-    let binary = fixture_dir.join("target/debug").join(binary_name);
+    let binary = target_dir.join(profile_dir).join(binary_name);
     assert!(binary.exists(), "Binary not found at {binary:?}");
     binary
 });

src/hyperlight-js-runtime/tests/native_modules.rs

@@ -193,14 +193,25 @@ fn build_js_runtime() -> PathBuf {
 }
 
 fn bundle_runtime() {
+    // Always rerun if the environment variable changes, even if it's currently unset.
+    println!("cargo:rerun-if-env-changed=HYPERLIGHT_JS_RUNTIME_PATH");
+
     let js_runtime_resource = match env::var("HYPERLIGHT_JS_RUNTIME_PATH") {
         Ok(path) => {
-            println!("cargo:warning=Using custom JS runtime: {}", path);
-            println!("cargo:rerun-if-env-changed=HYPERLIGHT_JS_RUNTIME_PATH");
-            println!("cargo:rerun-if-changed={}", path);
-            PathBuf::from(path)
+            let canonical = PathBuf::from(&path)
                 .canonicalize()
-                .expect("HYPERLIGHT_JS_RUNTIME_PATH must point to a valid file")
+                .expect("HYPERLIGHT_JS_RUNTIME_PATH must point to a valid file");
+            assert!(
+                canonical.is_file(),
+                "HYPERLIGHT_JS_RUNTIME_PATH must point to a file, not a directory: {}",
+                canonical.display()
+            );
+            println!(
+                "cargo:warning=Using custom JS runtime: {}",
+                canonical.display()
+            );
+            println!("cargo:rerun-if-changed={}", canonical.display());
+            canonical
         }
         Err(_) => build_js_runtime(),
     };