From cec44f5fc7565e0022e9cddb5357e0443270c8dd Mon Sep 17 00:00:00 2001
From: dluc <dluc@users.noreply.github.com>
Date: Tue, 21 Apr 2026 15:36:15 -0700
Subject: [PATCH] fix(node): await Promise returns from async JS hook handlers
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

JsHookHandlerBridge::handle reads the JS callback's return value as
String. When the handler is declared async, its return value is a
Promise, which napi-rs cannot coerce into String — the ThreadsafeFunction
layer raises a FATAL ERROR and the host process crashes.

Accept both sync (String) and async (Promise<String>) return types via
Either<String, Promise<String>>, awaiting the promise branch. This
matches the pattern already used by JsToolBridge::execute in
bindings/node/src/tools.rs.

The existing tests in __tests__/hooks.test.ts only exercised synchronous
handlers, which is why the regression wasn't caught. Added two new tests
covering (a) a simple async handler returning Continue and (b) an async
handler returning Deny with a subsequent handler that must not run.
The JsHookRegistry::register doc comment already advertised async
support (`(event, data) => string | Promise<string>`), so this change
makes the behavior match the documented contract.
---
 bindings/node/__tests__/hooks.test.ts | 40 +++++++++++++++++++++++++++
 bindings/node/src/hooks.rs            | 12 +++++++-
 2 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/bindings/node/__tests__/hooks.test.ts b/bindings/node/__tests__/hooks.test.ts
index 8ebd9ebf..c2ea420b 100644
--- a/bindings/node/__tests__/hooks.test.ts
+++ b/bindings/node/__tests__/hooks.test.ts
@@ -85,4 +85,44 @@ describe('JsHookRegistry', () => {
     expect(parsed).toHaveProperty('custom', 'value')
     expect(parsed).toHaveProperty('tool', 'grep')
   })
+
+  it('supports async handlers returning Promise<string>', async () => {
+    const registry = new JsHookRegistry()
+    let handlerCalled = false
+    let receivedEvent = ''
+
+    registry.register('tool:pre', async (event: string, _data: string) => {
+      handlerCalled = true
+      receivedEvent = event
+      // Simulate async work (e.g. I/O)
+      await new Promise<void>(resolve => setImmediate(() => resolve()))
+      return JSON.stringify({ action: 'continue' })
+    }, 10, 'async-hook')
+
+    const result = await registry.emit('tool:pre', '{"tool":"grep"}')
+
+    expect(handlerCalled).toBe(true)
+    expect(receivedEvent).toBe('tool:pre')
+    expect(result.action).toBe(HookAction.Continue)
+  })
+
+  it('async handler returning deny short-circuits pipeline', async () => {
+    const registry = new JsHookRegistry()
+    let secondRan = false
+
+    registry.register('tool:pre', async (_event: string, _data: string) => {
+      await new Promise<void>(resolve => setImmediate(() => resolve()))
+      return JSON.stringify({ action: 'deny', reason: 'async blocked' })
+    }, 10, 'async-deny')
+    registry.register('tool:pre', (_event: string, _data: string) => {
+      secondRan = true
+      return JSON.stringify({ action: 'continue' })
+    }, 20, 'after')
+
+    const result = await registry.emit('tool:pre', '{"tool":"rm"}')
+
+    expect(result.action).toBe(HookAction.Deny)
+    expect(result.reason).toBe('async blocked')
+    expect(secondRan).toBe(false)
+  })
 })
diff --git a/bindings/node/src/hooks.rs b/bindings/node/src/hooks.rs
index 91f6adb6..adcb729e 100644
--- a/bindings/node/src/hooks.rs
+++ b/bindings/node/src/hooks.rs
@@ -43,7 +43,10 @@ impl HookHandler for JsHookHandlerBridge {
             "{}".to_string()
         });
         Box::pin(async move {
-            let result_str: String =
+            // The JS handler may return either a bare string (synchronous) or a
+            // Promise<String> (async). `call_async` returns whatever the JS
+            // function returned — accept both via `Either`.
+            let ret: Either<String, napi::bindgen_prelude::Promise<String>> =
                 self.callback
                     .call_async((event, data_str))
                     .await
@@ -51,6 +54,13 @@ impl HookHandler for JsHookHandlerBridge {
                         message: e.to_string(),
                         handler_name: None,
                     })?;
+            let result_str: String = match ret {
+                Either::A(s) => s,
+                Either::B(promise) => promise.await.map_err(|e| HookError::HandlerFailed {
+                    message: e.to_string(),
+                    handler_name: None,
+                })?,
+            };
             let hook_result: HookResult = serde_json::from_str(&result_str).unwrap_or_else(|e| {
                 log::error!(
                     "SECURITY: Hook handler returned unparseable result — failing closed (Deny): {e} — json: {result_str}"