fix(node): await Promise returns from async JS hook handlers

[dluc]

TL;DR

The Node.js binding crashes the host process whenever an async JS hook handler is registered. Sync-only handlers work; any handler declared async — which is the only way to do I/O — triggers an unrecoverable FATAL ERROR from napi-rs, taking the whole process down. The fix is a 6-line change; the binding has been shipped with this bug since async support was advertised in the public API.

Reproduction (30 seconds)

This is a complete repro. Nothing unusual — a handler that awaits before returning:

import { JsHookRegistry } from 'amplifier-core'

const registry = new JsHookRegistry()

registry.register('tool:pre', async (event, data) => {
  // Any await — a DB write, a fetch, a fs read, a setImmediate…
  await new Promise(r => setImmediate(r))
  return JSON.stringify({ action: 'continue' })
}, 10, 'logger')

await registry.emit('tool:pre', '{"tool":"grep"}')

Actual result:

FATAL ERROR: threadsafe_function.rs:749 Failed to convert return value in
ThreadsafeFunction callback into Rust value: StringExpected, Failed to
convert JavaScript value `Object {}` into rust type `String`
----- Native stack trace -----
 1: node::OnFatalError
 2: napi_open_callback_scope
 3: amplifier-core.darwin-arm64.node
 4: v8impl::ThreadSafeFunction::AsyncCb
...

The process is gone. No exception to catch, no .catch() hook can save you — FATAL ERROR calls node::OnFatalError and exits.

Why this makes the binding unusable in practice

Every real-world hook needs to await something:

• activity-logger — writes to disk
• token-tracker — reads a store, updates it, persists
• approval-emitter — awaits a user decision via IPC
• status-context — fetches current session state
• todo-reminder — reads a file, injects context
• Any hook that calls a DB, HTTP endpoint, or filesystem

None of these are exotic — they're the canonical uses of hooks in the kernel's own design (pre/post tool, context injection, approval, logging). You cannot build any of them today without crashing the process. The only hooks that work are pure-compute transforms (count tokens already in memory, rewrite a string), which is a tiny fraction of what the hook system exists to do.

Result: the documented JsHookRegistry API is effectively dead — it works for toy examples and crashes for anything real.

Why this wasn't caught

Every handler in bindings/node/__tests__/hooks.test.ts is synchronous. The JSDoc on register() explicitly says both forms are supported:

/// (event: string, dataJson: string) => string | Promise<string>

…but the test matrix never covered the Promise case, and the Rust side never awaited it. The sibling JsToolBridge::execute in bindings/node/src/tools.rs handles the same pattern correctly — it awaits a Promise<String> from its JS callback. The hook bridge predates that pattern and was never updated. This PR brings the two into alignment.

Root cause

JsHookHandlerBridge::handle reads the JS callback's return value as String:

let result_str: String = self.callback.call_async(...).await?;

async functions return Promise, which napi-rs cannot coerce into String. The ThreadsafeFunction<_, ErrorStrategy::Fatal> treats conversion failure as a fatal error per its contract — hence the process crash rather than a recoverable error.

Fix (6 lines of real change)

Accept both sync and async return types via Either<String, Promise<String>>, awaiting the promise branch:

let ret: Either<String, Promise<String>> = self.callback.call_async(...).await?;
let result_str: String = match ret {
    Either::A(s) => s,
    Either::B(promise) => promise.await?,
};

Sync handlers continue to work unchanged — they take the Either::A path with no semantic or performance difference. Async handlers now work instead of crashing the process. This is exactly the pattern JsToolBridge::execute already uses.

Tests

Two new cases in __tests__/hooks.test.ts, each of which crashes the process on the unpatched binary:

1. supports async handlers returning Promise<string> — async handler returns Continue, verify no crash and result propagates.
2. async handler returning deny short-circuits pipeline — async Deny must prevent subsequent handlers from running (verifies the Deny precedence still holds through the async path).

Full suite after patch: 70/70 passing, including the 7 pre-existing hook tests.

Risk

Zero for existing users. Sync handlers take the same path they always did. Async handlers — which cannot work today — now work. This cannot regress any working code.

Severity

High. Any consumer of the Node binding attempting to write a production-grade hook (i.e. one that awaits) will hit this as their first integration test. The binding is advertising an API it can't deliver.

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Fixes a crash in the Node.js binding when hook handlers are async by correctly accepting and awaiting Promise<string> return values from JS hook callbacks.

Changes:

• Update the Rust hook bridge to accept Either<String, Promise<String>> from call_async and await the promise branch.
• Add Jest tests covering async hook handlers (continue and deny short-circuiting).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
bindings/node/src/hooks.rs	Accept and await promise returns from JS hook handlers to prevent fatal conversion crashes.
bindings/node/tests/hooks.test.ts	Adds regression tests for async hook handler behavior and pipeline short-circuiting.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

This change fixes the common async case, but any handler returning an unexpected type (e.g., an object) can still trigger a process-level fatal error if the underlying ThreadsafeFunction is configured with ErrorStrategy::Fatal (conversion into Either<...> would fail). Consider switching this bridge’s TSFN to a non-fatal strategy (e.g., CalleeHandled) and mapping conversion failures into HookError::HandlerFailed so user-land mistakes don’t hard-crash the host process.

[Copilot]

The HookError::HandlerFailed { message: e.to_string(), handler_name: None } mapping is duplicated (once for call_async(...).await and again for promise.await). To keep error handling consistent and reduce repetition, consider extracting this mapping into a small local closure/helper used in both places.

[Copilot]

This can be simplified to avoid an extra closure (e.g., pass resolve directly to setImmediate) while keeping the same behavior. It also makes the intent of ‘await one tick’ a bit clearer.

Suggested change

	await new Promise<void>(resolve => setImmediate(() => resolve()))
	await new Promise<void>(resolve => setImmediate(resolve))