Skip to content

Feat/webhook session key - #57972

Open
gilles007 wants to merge 3 commits into
NousResearch:mainfrom
gilles007:feat/webhook-session-key
Open

Feat/webhook session key#57972
gilles007 wants to merge 3 commits into
NousResearch:mainfrom
gilles007:feat/webhook-session-key

Conversation

@gilles007

Copy link
Copy Markdown

…conversations

What does this PR do?

Adds an opt-in session_key field to webhook route configuration, allowing a route to maintain a persistent conversation across deliveries.

Problem: The webhook platform derives the session chat key from the delivery ID (webhook:{route_name}:{delivery_id}), so every POST creates a brand-new session, and TTL prevents reuse. This is correct for fire-and-forget event routes, but makes multi-turn use cases impossible — each utterance from a voice satellite (my use case) arrives with total amnesia, and sessions multiply per event (the "ghost session" accumulation that v0.18.0's auto-close addressed).

Approach: Delivery and conversation identity are separated. A route may set session_key, a template rendered from the payload (e.g. "{satellite}"), which pins the chat key to a stable value: webhook:{route_name}:{rendered_key}. Repeat events from the same source then share one session, like a WhatsApp or Telegram thread. Idempotency still keys on delivery_id, and routes without session_key are completely unaffected — per-delivery remains the default. If the template doesn't resolve against a payload, the route falls back to per-delivery behavior for that event.

Because persistent routes expect follow-up turns, they are exempted from the on_processing_complete_end_webhook_session auto-close introduced in v0.18.0; their lifecycle is managed by idle timeout / hermes sessions prune instead. This does not reintroduce the ghost-session problem the auto-close fixed — session count is bounded by distinct rendered keys (one per satellite per route in my deployment), not by delivery count.

Security note: the session_key template is rendered from the request payload, so a sender controls which session their events land in. This is inherent to the feature and scoped to the route: a sender must already hold the route's HMAC secret, and cross-route access is not possible since the route name is part of the chat key. Signature validation and idempotency are unchanged.

Trade-off worth flagging: the original per-delivery key also served to keep concurrent POSTs on one route from serializing on a single session. With a shared session_key, rapid successive events from the same source will queue on one session. For conversational sources this is desirable (turns should be ordered); routes that need concurrency simply don't set session_key.

Related Issue

Fixes #

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • gateway/platforms/webhook.py — render optional session_key route config (template over payload) into the session chat key, falling back to delivery_id when unset or unresolved
  • gateway/platforms/webhook.py — exempt session_key routes from _end_webhook_session auto-close in on_processing_complete
  • <tests/docs/config-example paths — list what you actually added>

How to Test

  1. Add a route with a session key to ~/.hermes/config.yaml under platforms: webhook: extra::
   routes:
     voice-test:
       secret: ""
       session_key: "{satellite}"
       prompt: "[voice from {satellite}] {message}"
       deliver: log
  1. Restart the gateway and POST two signed events with the same satellite value:
   SECRET=""
   BODY='{"satellite":"assist_satellite.pod1","message":"My favorite color is teal. Remember it."}'
   SIG=$(printf '%s' "$BODY" | openssl dgst -sha256 -hmac "$SECRET" | awk '{print $2}')
   curl -si -X POST http://127.0.0.1:8644/webhooks/voice-test \
     -H "Content-Type: application/json" -H "X-Webhook-Signature: $SIG" -d "$BODY"
   # then a second POST asking "What is my favorite color?"
  1. Verify: both turns log the same session ID, the second shows non-empty history and the response recalls "teal", and hermes sessions list shows one new session, not two. A route without session_key still produces one session per delivery (unchanged default).

Tested end-to-end in a live deployment: voice satellites (Home Assistant → webhook) with per-satellite persistent sessions.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Linux spark-240 6.17.0-1008-nvidia Update snapshot id for ipython #8-Ubuntu SMP PREEMPT_DYNAMIC Wed Jan 21 17:56:56 UTC 2026 aarch64 aarch64 aarch64 GNU/Linux

Documentation & Housekeeping

  • [N/A] I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • [N/A] I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • [N/A] I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

Contents of ~/.hermes/voice-test.sh:
SECRET="same key as in config.yaml. ex: openssl rand -hex 24"
hermes-agent$ BODY='{"satellite":"assist_satellite.pod1","message":"My favorite color is teal. Remember it."}'
SIG=$(printf '%s' "$BODY" | openssl dgst -sha256 -hmac "$SECRET" | awk '{print $2}')
curl -si -X POST http://127.0.0.1:8644/webhooks/voice-test -H "Content-Type: application/json" -H "X-Webhook-Signature: $SIG" -d "$BODY"
sleep 35
BODY='{"satellite":"assist_satellite.pod1","message":"What is my favorite color?"}'
SIG=$(printf '%s' "$BODY" | openssl dgst -sha256 -hmac "$SECRET" | awk '{print $2}')
curl -si -X POST http://127.0.0.1:8644/webhooks/voice-test -H "Content-Type: application/json" -H "X-Webhook-Signature: $SIG" -d "$BODY"
sleep 35
hermes logs | grep -E "turn_context|Response for" | tail -4
hermes sessions list | head -4

gilles@spark-240:~/.hermes$ ./voice-test.sh
HTTP/1.1 202 Accepted
Content-Type: application/json; charset=utf-8
Content-Length: 97
Date: Fri, 03 Jul 2026 22:11:26 GMT
Server: Python/3.11 aiohttp/3.14.1

{"status": "accepted", "route": "voice-test", "event": "unknown", "delivery_id": "1783116686765"}HTTP/1.1 202 Accepted
Content-Type: application/json; charset=utf-8
Content-Length: 97
Date: Fri, 03 Jul 2026 22:12:01 GMT
Server: Python/3.11 aiohttp/3.14.1

{"status": "accepted", "route": "voice-test", "event": "unknown", "delivery_id": "1783116721786"}2026-07-03 12:10:31,853 INFO gateway.platforms.webhook: [webhook] Response for webhook:voice-test:assist_satellite.pod1: Oh, you're testing me?
2026-07-03 15:11:27,214 INFO [20260703_120854_0b7ee4f8] agent.turn_context: conversation turn: session=20260703_120854_0b7ee4f8 model=qwen3.6:27b provider=custom platform=webhook history=0 msg='[voice from assist_satellite.pod1] My favorite color is teal. Remember it.'
2026-07-03 15:11:54,685 INFO gateway.platforms.webhook: [webhook] Response for webhook:voice-test:assist_satellite.pod1: Teal. Consider it etched into my heart.
2026-07-03 15:12:01,847 INFO [20260703_120854_0b7ee4f8] agent.turn_context: conversation turn: session=20260703_120854_0b7ee4f8 model=qwen3.6:27b provider=custom platform=webhook history=2 msg='[voice from assist_satellite.pod1] What is my favorite color?'
Title Preview Last Active ID
──────────────────────────────────────────────────────────────────────────────────────────────────────────────
— [voice from assist_satellite.pod1] My just now 20260703_120854_0b7ee4f8
— What is my favorite color? 16h ago 20260702_225954_64eb4c4e

@alt-glitch alt-glitch added type/feature New feature or request comp/gateway Gateway runner, session dispatch, delivery platform/webhook Webhook / API server sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state P3 Low — cosmetic, nice to have labels Jul 3, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the focused persistent-webhook-session proposal; the current delivery-id session construction on main means the capability is still needed.

Problems

  • gateway/platforms/webhook.py:671-674 deliberately falls back to a delivery-specific chat ID when a template is unresolved, but :758-761 skips _end_webhook_session for every route that declares session_key. Those fallback one-shot sessions therefore violate the existing close/prune invariant.
  • gateway/platforms/webhook.py:674-687 uses the shared persistent chat ID as the _delivery_info key. send() reads response routing only from that key (gateway/platforms/webhook.py:277), while same-session events are queued in gateway/platforms/base.py:4675-4825; a later delivery can replace the earlier delivery's rendered destination before the earlier response is sent.

Suggested changes

  • Make lifecycle behavior depend on a successfully resolved persistent key for the specific event, not merely route configuration.
  • Keep response-routing state per delivery while separately deriving the stable conversation/session identity, and add pipeline coverage for both fallback cleanup and same-key deliveries with distinct deliver_extra.

Automated hermes-sweeper review.

row, so this never clobbers a ``compression``/``agent_close`` reason.
"""
route_name = (event.source.user_id or "").removeprefix("webhook:")
if self._routes.get(route_name, {}).get("session_key"):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This checks only whether the route declares session_key. If rendering at lines 671-673 leaves an unresolved token, line 674 falls back to a delivery-specific one-shot session but this return still prevents _end_webhook_session; that recreates the unprunable-session leak. Track whether this individual event actually resolved a persistent key.

session_key = self._render_prompt(session_key_tpl, payload, event_type, route_name).strip()
if "{" in session_key:
session_key = ""
session_chat_id = f"webhook:{route_name}:{session_key or delivery_id}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With a stable value this becomes the shared key for _delivery_info below. send() resolves deliver_extra only by chat ID, so a second same-key POST can overwrite the first delivery's response destination while the first run is active/queued. Keep response-routing state per delivery rather than using the conversation key for both roles.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/sessions Session lifecycle, resume, persistence, history labels Jul 15, 2026
@jzOcb

jzOcb commented Jul 25, 2026

Copy link
Copy Markdown

We have a workflow-webhook use case (agent-to-agent relay) and agree this capability is valuable. One caution from the current diff: the fallback delivery-id path still skips auto-close when the route declares session_key (leaking one-shot fallback sessions), and the shared _delivery_info key can let a later queued delivery overwrite an earlier event's rendered response target before that response sends. Separating conversation identity from per-delivery response routing would make this safe to adopt. Our narrower constant-per-route proposal is #71570 — it sidesteps both by requiring a stable configured delivery target, at the cost of not covering rendered per-payload identities like this PR does.

@GottZ GottZ left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was generated by AI during triage.

Summary

Two open PRs address persistent webhook conversations: #57972 adds rendered per-payload session identities plus lifecycle handling and documentation, while #71570 adds a narrower constant shared session per route. The current diffs do not yet safely resolve the reported cause: #57972 conflates conversation identity with delivery routing and mishandles fallback lifecycle, whereas #71570 changes only the chat ID and leaves persistent-session lifecycle unresolved.

Related pull requests

  • #57972 related — (+38/-1) — keep open, changes required: The rendered session_key supports both per-source persistent conversations and per-delivery fallback, but the diff skips auto-close based on route configuration rather than successful per-event resolution, leaking fallback sessions, and keys _delivery_info by the shared conversation ID, allowing queued deliveries to overwrite response routing. This follows the contributor keep_open review on #57972; merge requires separating stable conversation identity from per-delivery routing and making lifecycle conditional on actual key resolution.
  • #71570 related — (+13/-3) — superseded by #57972: The diff provides only one shared conversation per route and does not cover rendered per-payload identities; more importantly, it changes the session chat ID without adjusting the webhook completion lifecycle, so the shared session can still be ended after each delivery. Its constant shared key also does not separate conversation identity from mutable per-delivery response-routing state.

Duplicates

#57972 and #71570 substantially duplicate the opt-in persistent-webhook-session capability, but #57972 is the broader implementation because it can represent both route-level and per-payload identities after correction.

Suggested consolidation

Merge #57972 after addressing its contributor-reviewed lifecycle and response-routing blockers: track routing per delivery, derive conversation identity separately, and skip auto-close only when that delivery successfully resolved a persistent key. Then close #71570 as the narrower duplicate; its diff neither implements per-payload session partitioning nor completes the lifecycle changes needed for persistence.

Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 7 kB of PR diffs, 11 kB of issue/PR text, 2 kB of discussion (4 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

@GottZ GottZ left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was generated by AI during triage.

Delta since our previous triage comment

@teknium1’s new contributor review confirms and strengthens our earlier assessment of #71570: changing only the routing key does not preserve history because on_processing_complete() still closes the session, and the route-scoped _delivery_info key can misroute responses when deliveries overlap. The review adds authoritative code-path evidence but does not change the consolidation outcome.

Changed pull requests

  • #71570 duplicate — (+13/-3) — keep open for revision, but superseded for consolidation by #57972: @teknium1 confirms that the diff changes only the session key while leaving unconditional completion-time closure and shared-key response-routing aliasing unresolved. Despite the new keep_open review on #71570, consolidating into corrected #57972 remains preferable because #71570 is the narrower route-only design; this does not recommend closing it until #57972 has addressed the same lifecycle and routing blockers.

Suggested consolidation

The recommendation is unchanged: merge #57972 only after its reviewed lifecycle and routing fixes, then close #71570 as the narrower duplicate.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    subgraph Dup57972 ["PRs duplicating each other"]
        P57972["PR #57972 (open)"]
        P71570["PR #71570 (open)"]
    end
    class P57972 open
    class P71570 open
    class P57972 target
    click P57972 "https://github.com/NousResearch/hermes-agent/pull/57972"
    click P71570 "https://github.com/NousResearch/hermes-agent/pull/71570"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 7 kB of PR diffs, 11 kB of issue/PR text, 4 kB of discussion (6 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

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

Labels

area/sessions Session lifecycle, resume, persistence, history comp/gateway Gateway runner, session dispatch, delivery P3 Low — cosmetic, nice to have platform/webhook Webhook / API server sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants