Skip to content

Add Hermes Action Bus for typed WebUI/session actions - #3030

Closed
webflow-pt-carlos wants to merge 4 commits into
nesquena:masterfrom
webflow-pt:add-action-bus
Closed

webflow-pt-carlos wants to merge 4 commits into
nesquena:masterfrom
webflow-pt:add-action-bus

Conversation

@webflow-pt-carlos

Copy link
Copy Markdown

Summary

Adds a small typed backend Action Bus for Hermes WebUI session events.

The new dispatcher provides one shared primitive:

entry point  →  dispatch_action(name, payload, context)
             →  registered backend action
             →  ActionResult

This PR ships the primitive plus one trivial builtin, echo.test,
which round-trips the dispatch path without touching the session DB,
the agent, or the SSE channel. It exists so the bus can be exercised
end-to-end in unit tests and via the manual smoke test before any
session-touching action lands.

A follow-up PR adds session.nudge — the inference-only synthetic
user turn that wakes a session from a background trigger — together
with the load_visible_messages / append_assistant_message helpers
and the publish_session_message_appended SSE event that surfaces the
appended assistant message in open WebUI tabs.

Splitting the work this way keeps this PR mechanically tiny and lets
the session/agent integration land in its own focused review.

Motivation

Hermes WebUI already has several ways for non-typed-user events to
reach the system: WebUI interactions, cron jobs, webhooks, gateway
messages, and internal background jobs. Today, each entry point that
needs to trigger session-related work invents its own delivery and
execution flow (api/background.py, /background, /btw, ad-hoc
cron callbacks). Those entry points benefit from a shared typed
dispatcher that can:

  • run named backend actions consistently;
  • keep action handlers small and testable;
  • deduplicate repeated triggers with idempotency keys;
  • normalize success / error / silent-vs-visible into one result shape;
  • be wired into existing transports (HTTP, cron, gateway) without each
    caller reinventing the validation/auth/result layer.

This PR does not replace api/background.py. /background and
/btw remain the user-facing slash commands for "fork a
sub-conversation". A later PR can re-express those on top of the bus,
but that migration is out of scope here.

Design

Action Bus

The Action Bus is a registry plus dispatcher:

result = default_registry.dispatch(
    action="echo.test",
    payload={"content": "ping"},
    context=context,
    idempotency_key="smoke-2026-05-27T17-00",
)

Actions are registered by name and implement one method:

def run(self, payload: dict, context: ActionContext) -> ActionResult:
    ...

The bus is intentionally synchronous: Hermes WebUI runs on
ThreadingHTTPServer and the rest of api/* is plain def. Actions
follow the same style so dispatch can happen inside any POST handler
thread without spinning an event loop.

ActionResult

Every action returns the same shape:

ActionResult(
    ok=True,
    silent=False,
    assistant_message="ping",
    refresh_chat=False,
    meta={"source": "webui_api"},
)

The result separates three concerns:

  • ok: whether the action completed without unrecoverable error;
  • silent / assistant_message: whether anything should surface to chat;
  • refresh_chat: whether open clients should refresh session state.

Idempotency

ActionRegistry.dispatch accepts an optional idempotency_key. For a
given (action, idempotency_key) pair, the first successful (or
errored) result is cached for a TTL (default 300s) and returned for
repeat dispatches. The cache is in-memory only, guarded by a
threading.Lock to match the WebUI's threaded request model. A
process restart loses pending entries — acceptable for the v1 use
cases (uptime >> typical TTL) and a durable cache can slot in behind
the same dispatch signature in a later PR if needed.

echo.test (the only v1 builtin)

echo.test is the smallest possible action: it reads
payload["content"], returns it as assistant_message when
non-empty, and returns a silent ok otherwise. It does not touch the
session database, the agent, or the SSE channel. It exists so the
dispatch path can be exercised end-to-end in unit tests and via the
manual smoke test.

HTTP entry point

POST /api/actions accepts:

{
    "action": "echo.test",
    "payload": {"content": "ping"},
    "session_id": "...",         // optional
    "idempotency_key": "..."     // optional
}

and returns the dispatched ActionResult.to_dict() with HTTP status
200 on dispatch (regardless of ok), 400 on a malformed request, and
404 on an unknown action name. CSRF is enforced by the existing
api/routes.py::_check_csrf path; /api/actions is intentionally
NOT added to the CSRF-exempt allowlist — same-origin browser callers
work through the existing Origin/Host check, and non-browser callers
(curl, MCP, agent) pass through because they have no Origin header.

What lands in this PR

  1. api/actions/types.py — ActionContext, ActionResult, Action
    protocol, SILENT_SENTINEL.
  2. api/actions/registry.py — ActionRegistry, ActionNotFound,
    _IdempotencyCache, module-level default_registry.
  3. api/actions/__init__.py — package exports and register_builtins.
  4. api/actions/builtin/__init__.py
  5. api/actions/builtin/echo_test.py — the v1 builtin.
  6. api/actions_http.py — handle_actions_post(handler, body, registry, *, emit_event).
  7. api/routes.py — six-line route block routing /api/actions to
    the adapter, with lazy idempotent register_builtins against the
    process-global registry.
  8. tests/test_action_bus.py — 23 unit tests, no test-server fixture.

No new dependencies. No async runtime. No frontend changes (the SSE
event and frontend handler land in the follow-up PR alongside the
first real consumer).

Follow-up PRs

  • session.nudge builtin + the inference-only agent invocation helper
    • publish_session_message_appended SSE event + frontend handler
      (drafted alongside this PR).
  • Cron integration calling dispatch_action("session.nudge", ...).
  • Webhook integration calling dispatch_action(...).
  • Re-express /background and /btw on top of the bus.
  • Persistent audit table for action invocations.

Testing

  • 23 unit tests covering registry dispatch, idempotency (same key /
    different keys / no key), exception handling, the echo.test
    builtin (visible, silent, error), and the HTTP adapter (six 400
    cases, 404, 200, idempotency through the adapter, and emit_event
    pass-through). No test-server fixture required.

  • Manual smoke test against a running WebUI:

    curl -X POST http://127.0.0.1:8787/api/actions \
      -H "Content-Type: application/json" \
      -H "Cookie: <your hermes session cookie>" \
      -d '{
        "action": "echo.test",
        "idempotency_key": "smoke-test-1",
        "payload": {"content": "hello bus"}
      }'

    Expected: 200 with {"ok": true, "silent": false, "assistant_message": "hello bus", ...}. Repeat with the same idempotency_key → same body (cached). POST without action → 400. POST with an unregistered name → 404. Curl is a non-browser caller (no Origin header) so it passes CSRF naturally; browser callers go through the existing same-origin check.

Introduces a small typed backend Action Bus for Hermes session events.

The new dispatcher provides one shared primitive:

    entry point  ->  default_registry.dispatch(name, payload, context)
                 ->  registered backend action
                 ->  ActionResult

This PR ships the primitive plus one trivial builtin, echo.test, which
round-trips the dispatch path without touching the session database, the
agent, or the SSE channel. It exists so the bus can be exercised
end-to-end in unit tests and via the manual smoke test before any
session-touching action lands.

A follow-up PR adds session.nudge -- the inference-only synthetic user
turn that wakes a session from a background trigger -- together with the
load_visible_messages / append_assistant_message helpers and the
publish_session_message_appended SSE event that surfaces the appended
assistant message in open WebUI tabs.

Design choices:
- Synchronous to match the rest of api/* (ThreadingHTTPServer, plain def).
- No new dependencies, no frontend changes.
- Registry is injectable so per-test isolation needs no module-level resets.
- ActionNotFound -> 404, validation failures -> 400, dispatch -> 200.
- Idempotency cache is in-process, threading.Lock-guarded, TTL-pruned.
- CSRF is enforced via the existing _check_csrf path; /api/actions is NOT
  added to the exempt list.

Tests: tests/test_action_bus.py covers registry behavior, idempotency,
the echo.test builtin, and the HTTP adapter (validation, 404, 200,
idempotency, emit_event pass-through). 23 unit tests, no test-server
fixture required. Passes under both pytest and unittest.

Does not replace api/background.py's /background and /btw slash
commands; a later PR can re-express those on top of the bus.

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Claude <noreply@anthropic.com>
@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Summary

Read the full diff (8 files, +820), the new api/actions/{types,registry,builtin/echo_test}.py, the HTTP adapter api/actions_http.py, the route hook in api/routes.py:5022-5037, and all 23 tests in tests/test_action_bus.py. The shape is clean: a synchronous registry that fits the ThreadingHTTPServer model already in use across api/routes.py, a normalized ActionResult that decouples ok / silent / refresh_chat, an idempotency cache guarded by a threading.Lock, and a deliberately tiny v1 surface (one builtin, one POST route, no SSE side effects). Splitting session.nudge out into a follow-up PR is the right call — landing this primitive on its own keeps the review surface focused on the bus contract rather than the agent-invocation plumbing.

Things I verified

  • CSRF: /api/actions is correctly not added to _csrf_exempt_path at api/routes.py:1279-1286, so the existing _check_csrf at api/routes.py:1317 runs first. Curl callers without an Origin header pass through _is_browser_unsafe_request, browser callers go through the same-origin path. Matches the description.
  • read_body (helpers.py:363) returns {} on bad JSON, so the adapter's isinstance(body, dict) 400 path is only reachable if a future caller passes something non-dict directly. Tests cover that case via _MockHandler. Good.
  • The exception guard in ActionRegistry.dispatch at api/actions/registry.py:90-102:
try:
    result = impl.run(payload, context)
except Exception as exc:
    result = ActionResult(
        ok=False, silent=True,
        error=f"{type(exc).__name__}: {exc}",
    )

correctly catches Exception (not BaseException), so KeyboardInterrupt / SystemExit still propagate. ActionNotFound is raised before this guard, so it bubbles to the HTTP adapter's 404 mapping — confirmed by test_unknown_action_returns_404.

Three observations worth considering

1. Lazy registration check is fragile against future builtins. In api/routes.py:5026-5031:

if "echo.test" not in default_registry.known_actions():
    try:
        register_builtins(default_registry)
    except ValueError:
        pass

The sentinel is "echo.test". When the follow-up PR adds session.nudge, that check still passes (echo.test is already registered) so register_builtins won't run a second time to pick up the new builtin. Either swap the sentinel to "all of v1 is registered" semantics, or just rely on the ValueError-on-duplicate swallow and always call register_builtins(default_registry) — it's idempotent enough under the _register_lock. A small _BUILTINS_REGISTERED module flag inside api/actions/__init__.py would be cleaner.

2. ActionContext.dispatch is wired by the HTTP adapter but not by the test helper. In tests/test_action_bus.py:48-58, _ctx() builds an ActionContext without passing dispatch, so the default _no_dispatch (api/actions/types.py:26-31) raises RuntimeError if any test action tries to chain. v1 has no chaining actions so this is moot, but the follow-up session.nudge will likely want to chain (refresh, notify), and the test surface should grow _ctx(dispatch=registry.dispatch) at that point. Worth a sentence in the RFC.

3. _resolve_emit_event swallows ImportError for the missing publisher. At api/actions_http.py:24-37:

try:
    from api.session_events import publish_session_event
except ImportError:
    return lambda _name, _payload: None
return publish_session_event

api/session_events.py on origin/master only exposes publish_session_list_changed (grep -n "^def" api/session_events.py → one match at line 11). from api.session_events import publish_session_event against the current tree therefore raises ImportError and the no-op lambda is returned for every request until the follow-up PR adds the publisher. That's fine and intentional, but I'd add a comment at the import site noting that the symbol intentionally lands in a follow-up — readers grep'ing for publish_session_event today will hit one site (this one) and a no-op result, and they need to know that's expected rather than a typo.

Smaller nits

  • api/actions/registry.py:33-49 _IdempotencyCache._prune_locked iterates the full dict on every get. With low TTL (300s) and modest call volume that's cheap; if cron/webhook adapters later push thousands of keys/min, swap to a sorted insertion order pop. Not blocking for v1.
  • ActionResult.to_dict() always serializes error and refresh_chat; downstream JS will need to handle error: null. Fine, just flagging.
  • ActionResult.silent=True is the default. Easy to footgun if a future builtin returns a default ActionResult() and forgets to flip silent=False; echo.test correctly sets both. Maybe worth a runtime assertion in dispatch if assistant_message is non-empty but silent=True — but that's a stylistic choice.

Verdict

Foundation looks solid for the follow-up PRs. The three observations above are non-blocking polish; the only one I'd actually act on before landing is the registration sentinel in routes.py:5026 since it will silently bite the next builtin.

webflow-pt-carlos and others added 2 commits May 28, 2026 03:34
PR review observation #1 ("registration sentinel is fragile against
future builtins"):

The previous route hook in api/routes.py gated registration on
``"echo.test" not in default_registry.known_actions()``. When a
follow-up PR adds another builtin (e.g. session.nudge), that check
still passes because echo.test is already registered, so the new
builtin never gets picked up by register_builtins on a warm process.
The bug surfaces only after a deploy and is silent -- the new builtin
just returns 404.

Fix: track the registration call itself, not any individual action
name.

api/actions/__init__.py:
- Add module-level ``_BUILTINS_LOCK`` (threading.Lock) and
  ``_BUILTINS_REGISTERED`` flag.
- ``register_builtins(default_registry)`` is now idempotent: first
  call registers, later calls return immediately under the lock.
- ``register_builtins(custom_registry)`` -- the path tests take --
  is unchanged: registers fresh every call so per-test isolation
  still works.
- Pulled the actual registration list into a private
  ``_register_all_builtins(registry)`` helper so follow-up PRs only
  touch a single line to add their builtin; the locking and
  idempotency policy stays a clean wrapper around it.

api/routes.py:
- ``/api/actions`` hook is now a flat
  ``register_builtins(default_registry)`` call. Both the sentinel
  check and the ValueError swallow are gone.

api/actions_http.py:
- Per review observation #3, added an inline comment at the
  ``from api.session_events import publish_session_event`` site
  noting that the symbol intentionally lands in the session.nudge
  follow-up. Function-level docstring already covered the same; this
  is for grep readers.

tests/test_action_bus.py:
- New ``test_register_builtins_default_registry_is_idempotent``
  locks in the contract: repeated ``register_builtins(default_registry)``
  calls must not raise. Resets the module flag + clears the global
  registry inside the test so the assertion is deterministic.

Tests: 24/24 passing (was 23/23, +1 for the new idempotency test).

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Claude <noreply@anthropic.com>
Two changes, both within scope of PR #1 (the bus primitive) rather
than the session.nudge follow-up:

1. ``docs/rfcs/action-bus.md`` -- new RFC. The ``__init__.py``
   module docstring references this file as "added in this PR"
   but it was never actually shipped. The RFC documents the
   primitive's contract end-to-end:

   - Action / ActionContext / ActionResult shapes
   - Idempotency semantics and the in-process cache trade-offs
   - Error handling (caught Exception -> ActionResult; ActionNotFound
     bubbles)
   - Chaining via ``context.dispatch`` (the section the reviewer
     suggested adding "a sentence" to -- now a full subsection
     with the test reference)
   - Registration model (default_registry singleton + per-test
     registries + the lock-based idempotency from 3bd75bb)
   - HTTP entry point shape + CSRF posture
   - What's out of scope for v1 + planned follow-ups

2. ``tests/test_action_bus.py`` -- two new tests under
   ``TestEchoTest`` that lock in the chaining contract:

   - ``test_action_can_chain_via_context_dispatch`` -- a registered
     action chains to ``echo.test`` through ``context.dispatch``,
     wired via ``_ctx(dispatch=reg.dispatch)``. Proves the chain
     pattern works and exercises the ``_ctx(**overrides)``
     extensibility the reviewer flagged in observation #2.
   - ``test_chaining_without_dispatch_raises`` -- omitting the
     ``dispatch=`` override falls through to the
     ``_no_dispatch`` default, which raises ``RuntimeError`` that
     the registry catches and wraps into an error
     ``ActionResult``. Locks the failure mode in so a caller
     that forgets to opt into chaining gets a loud, structured
     error rather than silent wrong behavior.

The follow-up session.nudge PR no longer has to grow the test
surface for chaining; it can use the contract as documented and
tested here. Per the reviewer's observation #2 framing ("test
surface should grow ``_ctx(dispatch=registry.dispatch)`` at that
point") -- moving "that point" forward into this PR keeps the
follow-up's diff focused on the new action.

Tests: 26/26 passing (was 24/24, +2 chaining tests).

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Claude <noreply@anthropic.com>
@webflow-pt-carlos

webflow-pt-carlos commented May 28, 2026 •

Copy link
Copy Markdown
Author

Thanks for the thorough read. Acted on all three observations in two commits on this branch:

Observation 1 — fragile registration sentinel

Fixed in 3bd75bbb. Took the module-flag shape you suggested:

  • api/actions/__init__.py — added _BUILTINS_LOCK + _BUILTINS_REGISTERED at module level. Refactored register_builtins so calls against default_registry are idempotent (first call registers everything, subsequent calls return immediately under the lock); explicit-registry calls — the path tests take — are unchanged. Pulled the actual list of builtins into a private _register_all_builtins(registry) so follow-up PRs only touch one line; the locking policy stays a clean wrapper.
  • api/routes.py:5022-5037 — now a flat register_builtins(default_registry) call. No more sentinel, no ValueError swallow.
  • New test_register_builtins_default_registry_is_idempotent locks in the contract: the second call returns the same registry contents without raising.

The bug you described ("new builtin silently never picked up because echo.test sentinel still passes") is now structurally impossible — the flag tracks the registration call, not any individual action name, so any builtin added to _register_all_builtins in a follow-up PR is automatically picked up the first time the new process starts.

Observation 2 — chaining contract / test helper dispatch

Brought this one forward into this PR in 6b11c4b5 instead of deferring. Two changes:

  • docs/rfcs/action-bus.md — the RFC the __init__.py docstring already references as "added in this PR" but which actually wasn't shipped. The new RFC documents the bus contract end-to-end and includes a full Chaining section that explains context.dispatch, the _ctx(dispatch=registry.dispatch) test pattern, and the failure mode when dispatch= is omitted. (The sentence you asked for, expanded into the right amount of context for future readers.)
  • tests/test_action_bus.py — two new tests under TestEchoTest:
    • test_action_can_chain_via_context_dispatch — registered action chains to echo.test via context.dispatch, wired through _ctx(dispatch=reg.dispatch). Proves the chain pattern works and exercises the _ctx(**overrides) extensibility you pointed at.
    • test_chaining_without_dispatch_raises — omitting the dispatch= override hits the _no_dispatch default, raises RuntimeError, registry catches and wraps into an error ActionResult. Locks the failure mode in so a caller that forgets to opt into chaining gets a loud, structured error rather than silent wrong behavior.

This means the follow-up session.nudge PR no longer has to grow the test surface for chaining — the contract is documented and tested here.

Observation 3 — publish_session_event import comment

Added an inline comment at the import site in api/actions_http.py (same 3bd75bbb commit) so grep readers landing there on master understand the ImportError fallback is intentional. Function-level docstring already covered the same; this is the additional pointer you asked for.

Nits — acknowledged, not changed

Holding on these unless you want them in this PR:

  • Idempotency prune O(N): Noted. Will revisit if cron/webhook usage shows up in the hot path; for the v1 entry points (browser POST through /api/actions) it's well under the noise floor. The RFC documents the trade-off (Idempotency section) so future readers know it's a known tightening point.
  • error: null in ActionResult.to_dict(): Fine to surface; downstream JS in the follow-up handles error == null as "no error" rather than missing-key. The asymmetry was deliberate to keep the wire shape stable across success/failure — also now documented in the RFC.
  • silent=True default footgun: Fair point. Considered but not adding the runtime assertion in this PR (it'd be a behavior change worth its own review). Existing convention is "always set both ok and silent explicitly in any non-trivial action"; the RFC's ActionResult section calls this out. Happy to add the assertion if you want it before merge.

Final state

  • 26/26 unit tests passing in tests.test_action_bus (was 23/23, +3 new tests).
  • No behavior change for v1 callers, no API surface change.
  • New RFC documents the primitive end-to-end.
  • Ready for another look.
python3 -m unittest tests.test_action_bus -v

@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Re-review on 3bd75bb + 6b11c4b

Read both follow-up commits. All three observations are addressed cleanly, and the chaining work brought forward is a real improvement over deferring it.

Observation 1 — registration sentinel

The new shape in api/actions/__init__.py:45-89 separates the idempotency policy from the registration list:

if registry is default_registry:
    global _BUILTINS_REGISTERED
    with _BUILTINS_LOCK:
        if _BUILTINS_REGISTERED:
            return
        _register_all_builtins(registry)
        _BUILTINS_REGISTERED = True
else:
    _register_all_builtins(registry)

This fixes the silent-bug failure mode I flagged. The registry is default_registry identity check is the right discriminator — explicit per-test registries skip the flag entirely, so test isolation is preserved without resorting to fixtures. The route hook at api/routes.py:5022-5032 is now a flat call with no sentinel:

register_builtins(default_registry)

The "lock acquire per request" cost is GIL-pinned and well below the JSON parse cost the same handler already pays, so I don't think it's worth a _BUILTINS_REGISTERED fast-path before the lock. The check-under-lock idiom is correct.

Observation 2 — chaining contract

Pulling this forward into this PR is the right call. Two things I read carefully:

  • test_action_can_chain_via_context_dispatch at tests/test_action_bus.py:203-251 correctly exercises both halves: the chain producing chain -> ping proves dispatch wiring works, and the _ctx(dispatch=reg.dispatch) call proves _ctx(**overrides) accepts the extension point future builtins will need.

  • test_chaining_without_dispatch_raises at tests/test_action_bus.py:253-280 locks in the failure mode. Reading the registry's outer guard at api/actions/registry.py:90-102:

try:
    result = impl.run(payload, context)
except Exception as exc:
    result = ActionResult(
        ok=False, silent=True,
        error=f"{type(exc).__name__}: {exc}",
    )

so the RuntimeError from _no_dispatch surfaces as ok=False, silent=True, error="RuntimeError: ...". The assertion self.assertIn("RuntimeError", result.error or "") correctly anchors on the type name rather than the exact message, which keeps the test resilient against future message tweaks.

The RFC's Chaining section at docs/rfcs/action-bus.md:190-208 documents the contract, the source-tag propagation behavior, and the idempotency-key independence rule. The reference back to the specific test name (test_action_can_chain_via_context_dispatch) is a nice touch — it gives future readers a concrete entry point into the verified behavior.

Observation 3 — import comment

api/actions_http.py:35-41 now has the inline comment for grep readers:

# publish_session_event intentionally does not yet exist on this
# PR's branch -- it lands in the session.nudge follow-up. Grepping
# for the symbol on master today will only return this import; the
# ImportError fallback below resolves to a no-op for every request
# until then...

That's exactly the right scope. ✓

Nits

I'm not going to push on the four nits — your responses are reasonable. The silent=True default footgun assertion is the only one I'd genuinely lean toward, but you're right that it's a separate behavior change. RFC documents the convention; that's enough for v1.

One small thing on the idempotency test

test_register_builtins_default_registry_is_idempotent at lines 305-308 mutates module-private state (actions_pkg._BUILTINS_LOCK, _BUILTINS_REGISTERED, default_registry._actions) which is fine for a contract test, but it leaves the registry in a "first-run" state at the end of the test. If a later test in the same process invariant-checks default_registry.known_actions() expecting it to be empty (or expecting a specific set), this test order could matter. Worth a tearDown that re-clears state, or moving the reset into a setUp so the test is order-independent. Not blocking.

Verdict

Ready from my side. Foundation is solid for the session.nudge follow-up. 26 passing, two real bug-class fixes from the review (silent-no-pickup + chaining contract gap), and a proper RFC. Nice work.

Re-review nit from #3030: test_register_builtins_default_registry_is_idempotent
mutates module-private state (api.actions._BUILTINS_REGISTERED,
default_registry._actions) to observe a fresh registration pass.
The previous version left the registry in a "first-run" state after
the test, which would surprise any later test in the same process
that invariant-checks default_registry.known_actions().

Fix: snapshot the pre-test state and register an addCleanup() that
restores it. addCleanup is the unittest-canonical pattern -- it runs
even if assertions fail, so a partial test failure does not leak
state into subsequent tests.

This makes the test order-independent without needing a class-level
setUp/tearDown (only this one test mutates the global registry, and
keeping the snapshot/restore visible at the call site is clearer than
splitting it into framework hooks).

Tests: 26/26 still passing.

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Claude <noreply@anthropic.com>
@webflow-pt-carlos

Copy link
Copy Markdown
Author

Thanks for the re-review — appreciate the second careful read.

Acted on the order-independence concern in d484f4a2. The test now snapshots _BUILTINS_REGISTERED and default_registry._actions at entry and registers an addCleanup() to restore them on exit:

saved_registered = actions_pkg._BUILTINS_REGISTERED
saved_actions = dict(default_registry._actions)

def _restore_default_registry_state() -> None:
    with actions_pkg._BUILTINS_LOCK:
        actions_pkg._BUILTINS_REGISTERED = saved_registered
        default_registry._actions.clear()
        default_registry._actions.update(saved_actions)

self.addCleanup(_restore_default_registry_state)

Picked addCleanup over setUp/tearDown because only this one test mutates the global registry — keeping the snapshot/restore visible at the call site is clearer than scattering it into framework hooks for a class where the other tests don't touch global state. addCleanup also runs even if assertions fail, so a partial failure mid-test doesn't leak state into subsequent tests.

26/26 still passing. Branch tip is d484f4a2.

Thanks again for the thoughtful reviews — the two follow-up commits and this hygiene fix together turned the original "23 tests, working primitive" into "26 tests, two real bug-class fixes, full RFC, order-independent test suite." Better foundation for session.nudge to land on.

@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Re-review on d484f4a — test hygiene

Read the follow-up commit. The change at tests/test_action_bus.py:282-329 is exactly the right shape and lands cleanly on top of the previous two follow-ups.

What I verified

The order-independence story works end-to-end:

  1. Snapshot is taken before any mutation. saved_registered = actions_pkg._BUILTINS_REGISTERED and saved_actions = dict(default_registry._actions) run before the addCleanup registration, so the cleanup captures the pre-test state regardless of how many earlier tests in the same process have already exercised register_builtins.

  2. addCleanup registration happens before the destructive mutation. That ordering matters — if the test snapshotted, mutated, then registered the cleanup, an exception in the mutation block (e.g. KeyboardInterrupt mid-clear()) would leak the partial state. With self.addCleanup(...) ahead of the with actions_pkg._BUILTINS_LOCK: block at lines 314-317, the restore is guaranteed.

  3. The restore uses the same lock as the production registrar. api/actions/__init__.py:74-78 acquires _BUILTINS_LOCK to flip _BUILTINS_REGISTERED and populate the registry; the restore at lines 308-312 uses the same lock when undoing. Consistent locking discipline — concurrent registrars from other tests can't race the restore.

  4. dict(default_registry._actions) then .clear() + .update() preserves the registry identity. It's the same _actions dict any other code holding a reference would see, so order-dependence with respect to dict identity is also avoided.

On the choice of addCleanup over setUp/tearDown

The reasoning in the comment (only this one test mutates global state) is right. addCleanup keeps the snapshot/restore visible at the call site, which is a real readability win — a future contributor adding a builtins assertion to a sibling test won't have to scan a class-level tearDown to understand why the registry is empty between tests. The addCleanup-runs-on-assertion-failure property is also load-bearing here: a failed assertEqual(after_first, after_second) mid-test would otherwise leave the registry permanently with only the builtins set installed during the test.

Code I looked at

# api/actions/__init__.py:73-78
global _BUILTINS_REGISTERED
with _BUILTINS_LOCK:
    if _BUILTINS_REGISTERED:
        return
    ...
    _BUILTINS_REGISTERED = True

Matches the test's mutation/restore protocol — same lock, same flag, same _actions map.

Verdict

Approved from my side. Three review rounds, three substantive commits (3bd75bbb lock-based registration, 6b11c4b5 chaining contract + RFC, d484f4a2 test hygiene), and the original 23-test foundation grew into 26 with two real bug-class fixes uncovered by review. Good base for session.nudge to land on top of.

One micro-nit if you ever pass through again, not blocking: dict(default_registry._actions) is a shallow copy. If a future test mutates the per-action handler instance in place, the cleanup would restore the dict shape but not the action's internal state. Not relevant for the current EchoTestAction (no mutable per-instance state), but worth a one-line comment if the registry grows actions that carry per-instance counters or caches. Pure forward-looking note.

@webflow-pt-carlos

Copy link
Copy Markdown
Author

Closing the loop from my side — no further changes planned for this PR. Standing by for the CI workflow approval and merge whenever you're ready.

@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Triage: HOLD pending deep architectural review — labels: hold, maintainer-review

This introduces the Hermes Action Bus — a typed backend action dispatcher with a registry, a default-singleton registration helper, and an idempotency contract. That's a new always-visible architectural contract (the dispatch primitive becomes the canonical entry point for "typed WebUI/session actions"), so we want to think carefully about which layer it belongs in before it lands in hermes-webui.

Our standing rule for this kind of new architectural contract:

  • The action bus / typed-dispatcher primitive feels like it belongs in the hermes-agent layer (ContextEngine / memory-provider / plugin ABCs), not in WebUI. WebUI should be the thin consumer.
  • If WebUI gets its own action bus, we end up with two registry systems and two dispatch paths over time, with the agent-side bus eventually winning.

Before merging, we'd like to:

  1. Read docs/rfcs/action-bus.md carefully and decide whether the entry point belongs in hermes-agent. If it does, this work should land there first and WebUI imports it.
  2. If we keep it in WebUI for now (e.g., because the v1 surface is purely a route-side dispatcher and not used by the agent), be explicit about the deprecation/migration path when the agent gets its own.
  3. Confirm the v1 builtin (echo.test) really is the entire v1 surface and there isn't a hidden production caller in this PR.

Will follow up here after we've worked through the placement question. Thanks for the careful idempotency contract and the lock-around-default-registry pattern — that detail is good.

@webflow-pt-carlos

Copy link
Copy Markdown
Author

Thanks for the HOLD — and for the architectural read. Sitting with it, I think you're right, and I should have started in hermes-agent, not hermes-webui.

What we were actually trying to build

The Action Bus in this PR was not the end goal. The end goal was a session.nudge-style background turn (described in the PR as a follow-up): a caller (cron, webhook, etc.) sends a synthetic prompt into an existing session, Hermes runs a full inference turn (including tools if needed), and the user sees the assistant output — without the synthetic prompt appearing as a user message in the chat. Everything else from that turn (tool calls, results, side effects) should show and stay linked to the same session.

We split the work and landed the bus + echo.test first so the dispatch path could be reviewed separately. In hindsight that put the abstraction in the wrong repo: we were extending the WebUI we had open every day instead of looking at the engine underneath. That should have been obvious from the README's "thin consumer / CLI parity" framing — thank you for spelling it out.

On the bus vs the feature

After reading the agent codebase, I don't think the nudge feature needs a typed Action Bus at the agent layer either. Hermes already has the primitives we need: AIAgent.run_conversation, session persistence via SessionDB, and existing entry points (cron, gateway, CLI) that invoke the agent directly. The missing piece is an in-session pinch turn (renamed from "nudge" — the agent already uses "nudge" for empty-response retry logic): run inference on the live session, persist the full tool/assistant trace, but never persist or display the synthetic user prompt. No registry required for that.

The bus work here may still be useful as a reference if you decide a shared typed entry-point dispatcher belongs in hermes-agent someday — but that's your architectural call, not something we should ratify in WebUI. We're not trying to introduce a second dispatch path in the frontend.

Answering your checklist (3)

Yes: on this branch, echo.test is the only v1 builtin. There are no hidden production callers — only unit tests and the documented smoke-test curl. The session-touching path was intentionally left for the follow-up PR.

Next steps from our side

We plan to close this PR and re-propose against NousResearch/hermes-agent as a focused pinch primitive. WebUI would later be a thin consumer if upstream wants it.

Happy to leave docs/rfcs/action-bus.md in the branch for your reference, or drop it — whichever is cleaner for maintainers. Thanks again for the careful reviews on idempotency and registration; that helped even though the placement was wrong.

@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Thanks @webflow-pt-carlos — the typed dispatch design is thoughtful and the RFC is well-written. But we're going to decline this as-is: the PR is ~1290 lines (5 new modules + an HTTP endpoint + the 291-line RFC + 501 test lines) and the only thing it wires end-to-end is the echo.test self-test, which by design touches nothing (no session DB, agent, or SSE). The real value — session.nudge and the session/agent integration — is explicitly a follow-up PR.

We try hard to avoid merging speculative infrastructure ahead of its first real consumer: an abstraction we can't yet exercise against a concrete use case is hard to judge (does the shape fit? is the indirection worth it?) and becomes maintenance surface in the meantime. If you'd like to pursue this, please resubmit it bundled with the first real action (e.g. session.nudge) so the dispatcher and its consumer land together and we can evaluate the actual benefit vs. the complexity it adds. Closing for now — genuinely appreciate the design work, and we'd look again at a consumer-bundled version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hold maintainer-review Maintainer fit-assessment needed — may not merge even with fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants